-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve the interface of the comprehensive pagination engine #1358
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Improve the comprehensive pagination engine | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,15 +7,95 @@ export interface SimplePaginationEngine { | |
linkToPage: (pageNumber: number) => To | ||
} | ||
|
||
export interface PaginatedResults<Item> { | ||
/** | ||
* The data returned by a comprehensive pagination engine to the data consumer component | ||
*/ | ||
export interface ComprehensivePaginatedResults<Item, ExtractedData = typeof undefined> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is |
||
/** | ||
* Control interface that can be plugged to a Table's `pagination` prop | ||
*/ | ||
tablePaginationProps: TablePaginationProps | ||
|
||
/** | ||
* The data provided to the data consumer in the current window | ||
*/ | ||
data: Item[] | undefined | ||
|
||
/** | ||
* Any extra data produced by the transformer function (besides the array of items) | ||
*/ | ||
extractedData?: ExtractedData | undefined | ||
|
||
/** | ||
* Is the data set still loading from the server? | ||
*/ | ||
isLoading: boolean | ||
|
||
/** | ||
* Has the data been loaded from the server? | ||
*/ | ||
isFetched: boolean | ||
|
||
/** | ||
* Are we on the first page of the pagination? | ||
*/ | ||
isOnFirstPage: boolean | ||
|
||
/** | ||
* Do we have any data on the client page? | ||
*/ | ||
hasData: boolean | ||
|
||
/** | ||
* Can we say that there are no results at all? | ||
* | ||
* This is determined before any filtering or transformation. | ||
*/ | ||
hasNoResultsWhatsoever: boolean | ||
|
||
/** | ||
* Can we say that there are no results on the selected page | ||
* | ||
* This will only be marked as true if | ||
* - we are not the first page | ||
* - loading has finished | ||
*/ | ||
hasNoResultsOnSelectedPage: boolean | ||
Comment on lines
+56
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First sentence is redundant in most of these docs. What is the motivation for adding them? |
||
|
||
hasNoResultsBecauseOfFilters: boolean | ||
} | ||
|
||
export interface ComprehensivePaginationEngine<Item, QueryResult extends List> { | ||
selectedPage: number | ||
offsetForQuery: number | ||
limitForQuery: number | ||
paramsForQuery: { offset: number; limit: number } | ||
getResults: (queryResult: QueryResult | undefined, key?: keyof QueryResult) => PaginatedResults<Item> | ||
/** | ||
* A Comprehensive PaginationEngine sits between the server and the consumer of the data and does transformations | ||
* | ||
* Specifically, the interface for loading the data and the one for the data consumers are decoupled. | ||
*/ | ||
export interface ComprehensivePaginationEngine< | ||
Item, | ||
QueryResult extends List, | ||
ExtractedData = typeof undefined, | ||
> { | ||
/** | ||
* The currently selected page from the data consumer's POV | ||
*/ | ||
selectedPageForClient: number | ||
|
||
/** | ||
* Parameters for data to be loaded from the server | ||
*/ | ||
paramsForServer: { offset: number; limit: number } | ||
|
||
/** | ||
* Get the current data/state info for the data consumer component. | ||
* | ||
* @param isLoading Is the data still being loaded from the server? | ||
* @param queryResult the data coming in the server, requested according to this engine's specs, including metadata | ||
* @param key The field where the actual records can be found within queryResults | ||
*/ | ||
getResults: ( | ||
isLoading: boolean, | ||
isFetched: boolean, | ||
queryResult: QueryResult | undefined, | ||
key?: keyof QueryResult, | ||
) => ComprehensivePaginatedResults<Item, ExtractedData> | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,14 +1,58 @@ | ||||||
import { To, useSearchParams } from 'react-router-dom' | ||||||
import { AppErrors } from '../../../types/errors' | ||||||
import { ComprehensivePaginationEngine } from './PaginationEngine' | ||||||
import { ComprehensivePaginatedResults, ComprehensivePaginationEngine } from './PaginationEngine' | ||||||
import { List } from '../../../oasis-nexus/api' | ||||||
import { TablePaginationProps } from './TablePagination' | ||||||
|
||||||
type ClientSizePaginationParams<Item> = { | ||||||
type Filter<Item> = (item: Item) => boolean | ||||||
|
||||||
type ClientSizePaginationParams<Item, QueryResult extends List, ExtractedData = typeof undefined> = { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
/** | ||||||
* How should we call the query parameter (in the URL)? | ||||||
*/ | ||||||
paramName: string | ||||||
Comment on lines
+10
to
13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where selected page is stored? |
||||||
|
||||||
/** | ||||||
* The pagination page size from the POV of the data consumer component | ||||||
*/ | ||||||
clientPageSize: number | ||||||
|
||||||
/** | ||||||
* The pagination page size used for actually loading the data from the server. | ||||||
* | ||||||
* Please note that currently this engine doesn't handle when the data consumer requires data which is not | ||||||
* part of the initial window on the server side. | ||||||
Comment on lines
+23
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand |
||||||
*/ | ||||||
serverPageSize: number | ||||||
filter?: (item: Item) => boolean | ||||||
|
||||||
/** | ||||||
* Filter to be applied to the loaded data. | ||||||
* | ||||||
* This is the order of processing: | ||||||
* - transform() | ||||||
* - filter | ||||||
* - filters | ||||||
* - order */ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "order" doesn't exist |
||||||
filter?: Filter<Item> | undefined | ||||||
|
||||||
/** | ||||||
* Filter to be applied to the loaded data. | ||||||
* | ||||||
* This is the order of processing: | ||||||
* - transform() | ||||||
* - filter | ||||||
* - filters | ||||||
* - order | ||||||
*/ | ||||||
filters?: (Filter<Item> | undefined)[] | ||||||
|
||||||
/** | ||||||
* Transformation to be applied after loading the data from the server, before presenting it to the data consumer component | ||||||
* | ||||||
* Can be used for ordering, aggregation, etc.D | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo |
||||||
* If both transform and filter is set, transform will run first. | ||||||
*/ | ||||||
transform?: (input: Item[], results: QueryResult) => [Item[], ExtractedData] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return type is very limiting - "transform" implies you can return items in a different format. But correctly typing transformations is difficult - it should allow returning items with a new type. And that new type should be passed into filters There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is an actual usecase for extractedData? |
||||||
} | ||||||
|
||||||
const knownListKeys: string[] = ['total_count', 'is_total_count_clipped'] | ||||||
|
@@ -27,13 +71,21 @@ function findListIn<T extends List, Item>(data: T): Item[] { | |||||
} | ||||||
} | ||||||
|
||||||
export function useClientSidePagination<Item, QueryResult extends List>({ | ||||||
/** | ||||||
* The ClientSidePagination engine loads the data from the server with a big window in one go, for in-memory pagination | ||||||
*/ | ||||||
export function useClientSidePagination<Item, QueryResult extends List, ExtractedData = typeof undefined>({ | ||||||
paramName, | ||||||
clientPageSize, | ||||||
serverPageSize, | ||||||
filter, | ||||||
}: ClientSizePaginationParams<Item>): ComprehensivePaginationEngine<Item, QueryResult> { | ||||||
const selectedServerPage = 1 | ||||||
filters, | ||||||
transform, | ||||||
}: ClientSizePaginationParams<Item, QueryResult, ExtractedData>): ComprehensivePaginationEngine< | ||||||
Item, | ||||||
QueryResult, | ||||||
ExtractedData | ||||||
> { | ||||||
const [searchParams] = useSearchParams() | ||||||
const selectedClientPageString = searchParams.get(paramName) | ||||||
const selectedClientPage = parseInt(selectedClientPageString ?? '1', 10) | ||||||
|
@@ -57,30 +109,51 @@ export function useClientSidePagination<Item, QueryResult extends List>({ | |||||
return { search: newSearchParams.toString() } | ||||||
} | ||||||
|
||||||
const limit = serverPageSize | ||||||
const offset = (selectedServerPage - 1) * clientPageSize | ||||||
// From the server, we always want to load the first batch of data, with the provided (big) window. | ||||||
// In theory, we could move this window as required, but currently this is not implemented. | ||||||
const selectedServerPage = 1 | ||||||
Comment on lines
+112
to
+114
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is batch a different concept from window and page? |
||||||
|
||||||
// The query parameters that should be used for loading the data from the server | ||||||
const paramsForQuery = { | ||||||
offset, | ||||||
limit, | ||||||
offset: (selectedServerPage - 1) * serverPageSize, | ||||||
limit: serverPageSize, | ||||||
} | ||||||
|
||||||
return { | ||||||
selectedPage: selectedClientPage, | ||||||
offsetForQuery: offset, | ||||||
limitForQuery: limit, | ||||||
paramsForQuery, | ||||||
getResults: (queryResult, key) => { | ||||||
const data = queryResult | ||||||
? key | ||||||
? (queryResult[key] as Item[]) | ||||||
: findListIn<QueryResult, Item>(queryResult) | ||||||
selectedPageForClient: selectedClientPage, | ||||||
paramsForServer: paramsForQuery, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename both to paramsForServer? |
||||||
getResults: ( | ||||||
isLoading, | ||||||
isFetched, | ||||||
queryResult, | ||||||
key, | ||||||
): ComprehensivePaginatedResults<Item, ExtractedData> => { | ||||||
const data = queryResult // we want to get list of items out from the incoming results | ||||||
? key // do we know where (in which field) to look? | ||||||
? (queryResult[key] as Item[]) // If yes, just get out the data | ||||||
: findListIn<QueryResult, Item>(queryResult) // If no, we will try to guess | ||||||
: undefined | ||||||
const filteredData = !!data && !!filter ? data.filter(filter) : data | ||||||
|
||||||
// Apply the specified client-side transformation | ||||||
const [transformedData, extractedData] = !!data && !!transform ? transform(data, queryResult!) : [data] | ||||||
|
||||||
// Select the filters to use. (filter field, filters field, drop undefined ones) | ||||||
const filtersToApply = [filter, ...(filters ?? [])].filter(f => !!f) as Filter<Item>[] | ||||||
|
||||||
// Apply the specified filtering | ||||||
const filteredData = transformedData | ||||||
? filtersToApply.reduce<Item[]>( | ||||||
(partiallyFiltered, nextFilter) => partiallyFiltered.filter(nextFilter), | ||||||
transformedData, | ||||||
) | ||||||
: transformedData | ||||||
|
||||||
// The data window from the POV of the data consumer component | ||||||
const offset = (selectedClientPage - 1) * clientPageSize | ||||||
const limit = clientPageSize | ||||||
const dataWindow = filteredData ? filteredData.slice(offset, offset + limit) : undefined | ||||||
|
||||||
// The control interface for the data consumer component (i.e. Table) | ||||||
const tableProps: TablePaginationProps = { | ||||||
selectedPage: selectedClientPage, | ||||||
linkToPage, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you improve the comment below?
it doesn't say TODO, it doesn't say what's wrong, and repeats last line? |
||||||
|
@@ -93,11 +166,25 @@ export function useClientSidePagination<Item, QueryResult extends List>({ | |||||
isTotalCountClipped: queryResult?.is_total_count_clipped, // TODO | ||||||
rowsPerPage: clientPageSize, | ||||||
} | ||||||
|
||||||
const isOnFirstPage = tableProps.selectedPage === 1 | ||||||
const hasData = !!dataWindow?.length | ||||||
const hasNoResultsOnSelectedPage = !isLoading && !isOnFirstPage && !hasData | ||||||
const hasNoResultsWhatsoever = !isLoading && !queryResult?.total_count | ||||||
const hasNoResultsBecauseOfFilters = !isLoading && !!transformedData?.length && !filteredData?.length | ||||||
|
||||||
return { | ||||||
tablePaginationProps: tableProps, | ||||||
data: dataWindow, | ||||||
extractedData, | ||||||
isLoading, | ||||||
isFetched, | ||||||
hasData, | ||||||
isOnFirstPage, | ||||||
hasNoResultsWhatsoever, | ||||||
hasNoResultsOnSelectedPage, | ||||||
hasNoResultsBecauseOfFilters, | ||||||
} | ||||||
}, | ||||||
// tableProps, | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filename typo