-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[DataGridPro] Server side data source lazy loading #13878
base: master
Are you sure you want to change the base?
[DataGridPro] Server side data source lazy loading #13878
Conversation
Deploy preview: https://deploy-preview-13878--material-ui-x.netlify.app/ Updated pages: |
a16525b
to
9302410
Compare
3e7e817
to
f747d05
Compare
3d0de1a
to
04bff9b
Compare
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.
Really nice initial implementation 👍
Thank you for picking this up. 🙏
packages/x-data-grid-pro/src/hooks/features/dataSource/useGridDataSource.ts
Outdated
Show resolved
Hide resolved
packages/x-data-grid-pro/src/hooks/features/serverSideLazyLoader/useGridDataSourceLazyLoader.ts
Outdated
Show resolved
Hide resolved
packages/x-data-grid-pro/src/hooks/features/dataSource/useGridDataSource.ts
Outdated
Show resolved
Hide resolved
2aaa688
to
99cd596
Compare
9257ca3
to
5eefb3d
Compare
6df3bb1
to
eae88f2
Compare
fcecea3
to
9230211
Compare
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.
Nice job combining lazy and infinite loading and explaining this well in the docs!
docs/data/data-grid/server-side-data/ServerSideLazyLoadingModeUpdate.tsx
Show resolved
Hide resolved
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.
Do you think we can keep showing previous data while loading? Like in https://mui.com/x/react-data-grid/server-side-data/#server-side-filtering-sorting-and-pagination
This seems to be a nicer UX to me.
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.
You mean when the row count is changing?
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.
Sorry, I forgot to mention: for example, when sort changes
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.
It is a bit more complicated than for the paginated data fetch
With pagination all rows are replaced with every new response
With lazy loading, fetched rows have to be injected at certain position.
At the moment, lazy loading hook only generates parameters for the data source which then based on the flag either sets new row list or replaces rows from certain index.
This means that the list cannot be kept, since it can happen that one row gets rendered twice (raising an error) before lazy loading hook can clean things up.
I agree that the grid looks too empty after user actions, so I see two options here:
- Instead of clearing the grid, replace all rows with skeletons for the actions that need to fetch the first page again (filter change, invalid row count, etc..) and update the
addSkeletonRows
function to also do the cleanup, since the row count will probably decrease after filtering - Change the way new rows are added and move that responsibility to the lazy loading hook, which means that the data source hook will either replace the rows or send that as an event data to be handled elsewhere. In that case, lazy loading hook can clean things up before adding those rows
I am more for the option 1, but open for other suggestions
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.
I thought a bit more about this and I think that the best way to do this is to add additional prop called unstable_onDataSourceResolve
that can be used to intercept and manipulate the way the new data is added to the grid. Default would be setRows(getRowsResponse.rows)
.
There are couple of benefits:
- lazy loading can use this to replace row indexes while keeping the old rows until the new rows are ready.
- it gives developers the flexibility to use this for some other use cases they might have
- we don't have to have any lazy loading logic inside the dataSource hook
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.
If the intention is to decouple the grid row tree updates from the useGridDataSource
hook and delegate it to the respective feature hooks, it makes sense to me. I even planned to do it in the very first PR but couldn't implement it back then.
Now that things have started to get a bit more complex, I would love to adapt that approach, but I'd prefer to use an event-emitting approach instead of adding a prop.
A high-level view of what I have in my mind:
const useGridDataSource = (...params) => {
// otherCode
const fetchRows() => {
const getRowsParams = getRowsParamsSelector();
const cachedData = cache.get(getRowsParams);
if (cachedData) {
// Event name could be something else `rowsResolved` could be an option too
apiRef.current.publishEvent('rowsFetched', { params: getRowsParams, data: cachedData });
return;
}
const fetchedData = dataSource.getRows(getRowsParams);
cache.set(getRowsParams, fetchedData);
apiRef.current.publishEvent('rowsFetched', { params: getRowsParams, data: fetchedData });
}
const fetchChildren = (...args) => {
// similar logic but the published event could be different, something like `childRowsFetched`
}
}
Now listening to each specific event will be delegated to the specific hook. For example useGridRows
could listen to rowsFetched
and handle apiRef.current.setXXX
stuff. In the case of props.lazyLoading='true'
, useGridLazyLoader
could take that role.
For childRowsFetched
, either useGridTreeData
or useGridRowGrouping
would come into action based on treeData
prop and row grouping state.
A few benefits to this approach:
- Simplify the parent hook, keep only the generic stuff (useGridDataSource)
- More code relevance (for example, code referring to the row grouping will be in the row grouping hook and in the premium package)
- Scalable and easier to support exceptional cases like the one in discussion on this thread.
- Easier customizability in the userland, since the users can listen and respond to these events too.
What do you think about it? We could even extract this change separately from this PR to make things smoother.
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.
Not sure if it is just a coincidence or you used it as an example, but there is already an event rowsFetched
that I have added to respond to the data change in the lazy loading hook
useGridApiEventHandler(privateApiRef, 'rowsFetched', handleDataUpdate);
Also in my previous comment point 2 is what you have described with a bit more detail in your comment.
I am perfectly fine with extending this event to pass the relevant data as well
I think the important point is
Easier customizability in the userland, since the users can listen and respond to these events too.
there will probably be some other events and actions that have to work together with the data source in the user apps
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.
Cool, nah I overlooked that.
So using the event with data could resolve the need for unstable_onDataSourceResolve
prop, right?
Also in my previous comment point 2 is what you have described with a bit more detail in your comment.
Yeah mostly aligned with the point no 2 you mentioned with difference that the rows tree updates would be handled by respective hooks rather than data source hook + other hooks.
But yeah, I'd be in favour of this proposal. 👍
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.
Not sure I follow how these events could be used by the users...
Can you give some examples?
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.
So using the event with data could resolve the need for unstable_onDataSourceResolve prop, right?
yes, the good point is that the data source hook does not have to worry about how the data is rendered, but the bad part is that each hook has to know about the other hook to know when it is its turn to render (useGridRows
has to check lazyLoading
flag)
To answer on
Not sure I follow how these events could be used by the users...
Can you give some examples?
I was thinking
You can send the event to retry the request (this is already in place in the error demo)
You can listen to the event to update UI outside of the grid (if needed)
with resolve callback, you get even more control since you can intercept and completely stop the rendering (with the events, there will always be at least one hook that processes the data)
Now, to answer to
What would be the default behavior for unstable_onDataSourceResolve?
Default would be to setRows(getRowsResponse.rows)
like it does now.
Passing a function would mean that the data source hook will not do anything, but just pass the result. User can choose to process it a bit further and then (partially) render it or even discard it completely.
also, since we already have onDataSourceError
, it doesn't seem strange to have the resolve callback as well.
f50f6de
to
14cd89d
Compare
cc512ee
to
bcf300f
Compare
firstRowToRender: `${params.start}`, | ||
lastRowToRender: `${params.end}`, |
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.
Could we use similar terminology in the mock server to keep things more consistent?
firstRowToRender: `${params.start}`, | |
lastRowToRender: `${params.end}`, | |
start: params.start, | |
end: params.end, |
|
||
function ServerSideLazyLoadingErrorHandling() { | ||
const apiRef = useGridApiRef(); | ||
const [retryParams, setRetryParams] = React.useState<GridGetRowsParams | null>( |
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.
Is it possible to hide the Alert when the data is fetched? Maybe by using rowsFetched
event?
error-demo.mp4
That being said, I think passing getRowsParams
as additional information with the rowsFetched
event could be useful.
What do you think?
|
||
Based on the previous and the new value for the total row count, the following scenarios are possible: | ||
|
||
- **Unknown `rowCount` to known `rowCount`**: If row count is not unknown anymore, the grid switches to the viewport loading mode. It checks the amount of allready fetched rows and adds skeleton rows to match the total row count. |
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.
- **Unknown `rowCount` to known `rowCount`**: If row count is not unknown anymore, the grid switches to the viewport loading mode. It checks the amount of allready fetched rows and adds skeleton rows to match the total row count. | |
- **Unknown `rowCount` to known `rowCount`**: When the row count is set to a valid value from an unknown value, the Data Grid switches to the viewport loading mode. It checks the number of already fetched rows and adds skeleton rows to match the provided row count. |
|
||
- **Unknown `rowCount` to known `rowCount`**: If row count is not unknown anymore, the grid switches to the viewport loading mode. It checks the amount of allready fetched rows and adds skeleton rows to match the total row count. | ||
|
||
- **Known `rowCount` to unknown `rowCount`**: If the row count is updated and set to `-1`, the grid resets, fetches the first page and sets itself in the infinite loading mode. |
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.
- **Known `rowCount` to unknown `rowCount`**: If the row count is updated and set to `-1`, the grid resets, fetches the first page and sets itself in the infinite loading mode. | |
- **Known `rowCount` to unknown `rowCount`**: If the row count is updated and set to `-1`, the Data Grid resets, fetches the first page, and sets itself in the infinite loading mode. |
|
||
If the user scrolls too fast, the grid loads multiple pages with one request (by adjusting `start` and `end` param) in order to reduce the server load. | ||
|
||
In addition to this, the grid throttles new requests made to the data source after each rendering context change. This can be controlled with `lazyLoadingRequestThrottleMs` prop. |
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.
Would it be useful to add a sub-section with a demo to demonstrate lazyLoadingRequestThrottleMs
prop?
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.
Is it possible to avoid showing stale rows before updating with the new (probably extend the loading until the rows have been replaced)?
stale.mp4
::: | ||
|
||
When completed, it will be possible to use `lazyLoading` flag in combination with [Tree data](/x/react-data-grid/server-side-data/tree-data/) and [Row grouping](/x/react-data-grid/server-side-data/row-grouping/). |
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.
To fix vale warning
When completed, it will be possible to use `lazyLoading` flag in combination with [Tree data](/x/react-data-grid/server-side-data/tree-data/) and [Row grouping](/x/react-data-grid/server-side-data/row-grouping/). | |
When completed, it would be possible to use `lazyLoading` flag in combination with [Tree data](/x/react-data-grid/server-side-data/tree-data/) and [Row grouping](/x/react-data-grid/server-side-data/row-grouping/). |
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.
How about deprecating the existing infinite loading
and lazy loading
in favor of data source infinite loading and viewport loading?
Or do they still make sense for some use-cases not possible with data source based implementation?
unstable_dataSource={dataSource} | ||
lazyLoading | ||
paginationModel={{ page: 0, pageSize: 10 }} | ||
rowCount={rowCount} |
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.
Is it possible to support the estimatedRowCount
prop with the lazy-loading? 🤔
I feel it could be useful in some usecases.
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.
Server-side sorting
, filtering
, pagination
, etc. doesn't seem to work in the existing demos.
Tested with plain-data, tree-data, row grouping
Working on live versions: https://mui.com/x/react-data-grid/server-side-data/
Is it because of the usage of firstRowToRender
, lastRowToRender
in useMockServer
?
Maybe simplifying it to the same interface as GridDataSource
would be better?
|
||
Initially, the first page data is fetched and displayed in the grid. What triggers the loading of next page data depends on the value of the total row count. | ||
|
||
If the total row count is known, the grid gets filled with skeleton rows and fetches more data if one of the skeleton rows falls into the rendering context. |
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.
IIRC, we prefer to use full product names rather than the shortened ones.
CC @samuelsycamore
If the total row count is known, the grid gets filled with skeleton rows and fetches more data if one of the skeleton rows falls into the rendering context. | |
If the total row count is known, the Data Grid gets filled with skeleton rows and fetches more data if one of the skeleton rows falls into the rendering context. | |
This loading strategy is often referred to as [**viewport loading**](#viewport-loading). |
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.
+1 - you could also just write "Grid" if it's too repetitive to say "Data Grid" over and over, but either way it's preferable to capitalize it to make it clear that it's the MUI X product specifically.
|
||
If the total row count is known, the grid gets filled with skeleton rows and fetches more data if one of the skeleton rows falls into the rendering context. | ||
|
||
If the total row count is unknown, the grid fetches more data when the user scrolls to the bottom of the grid. This loading strategy is often referred to as **infinite loading**. |
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.
If the total row count is unknown, the grid fetches more data when the user scrolls to the bottom of the grid. This loading strategy is often referred to as **infinite loading**. | |
If the total row count is unknown, the Data Grid fetches more data when the user scrolls to the bottom. This loading strategy is often referred to as [**infinite loading**](#infinite-loading). |
If the total row count is unknown, the grid fetches more data when the user scrolls to the bottom of the grid. This loading strategy is often referred to as **infinite loading**. | ||
|
||
:::info | ||
Row count can be provided either by returning the `rowCount` in the response of the `getRows` method in `unstable_dataSource`, via the `rowCount` prop or by calling [`setRowCount`](/x/api/data-grid/grid-api/#grid-api-prop-setRowCount) API. |
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.
Nitpick: Might combine two callouts into one.
Row count can be provided either by returning the `rowCount` in the response of the `getRows` method in `unstable_dataSource`, via the `rowCount` prop or by calling [`setRowCount`](/x/api/data-grid/grid-api/#grid-api-prop-setRowCount) API. | |
Row count can be provided in either of the following ways. | |
- Pass as `rowCount` prop | |
- Return `rowCount` by the `getRows` method | |
- Set the `rowCount` using the [`setRowCount`](/x/api/data-grid/grid-api/#grid-api-prop-setRowCount) API method. | |
The above list is given in the order of precedence, which means if the row count is set using the API, that value gets overridden once a new value is returned by the `getRows` method, even if it is `undefined`. |
:::warning | ||
Order of precedence for the row count: | ||
|
||
- `rowCount` prop | ||
- `rowCount` returned by the `getRows` method | ||
- row count set using the `setRowCount` API | ||
|
||
This means that, if the row count is set using the API, that value gets overridden once a new value is returned by the `getRows` method, even if it is `undefined`. | ||
::: |
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.
Linked with the other comment.
:::warning | |
Order of precedence for the row count: | |
- `rowCount` prop | |
- `rowCount` returned by the `getRows` method | |
- row count set using the `setRowCount` API | |
This means that, if the row count is set using the API, that value gets overridden once a new value is returned by the `getRows` method, even if it is `undefined`. | |
::: |
- `rowCount` returned by the `getRows` method | ||
- row count set using the `setRowCount` API | ||
|
||
This means that, if the row count is set using the API, that value gets overridden once a new value is returned by the `getRows` method, even if it is `undefined`. |
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.
even if it is
undefined
Does that mean that even if there's no rowCount
returned from the getRows
call (equivalent to being undefined
), the previous value set using the API method will be reset?
Wouldn't that be against the users' expectations? 🤔
|
||
The viewport loading mode is enabled when the row count is known (`rowCount >= 0`). Grid fetches the first page immediately and adds skeleton rows to match the total row count. Other pages are fetched once the user starts scrolling and moves a skeleton row inside the rendering context (index range defined by [Virtualization](/x/react-data-grid/virtualization/)). | ||
|
||
If the user scrolls too fast, the grid loads multiple pages with one request (by adjusting `start` and `end` param) in order to reduce the server load. |
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.
I'd love to have a section about the chunking we do to optimize the caching and reduce backend calls for the lazy loading use cases. Do you think it makes sense to add one in first iteration?
return; | ||
} | ||
|
||
// with lazy loading, only the initial load should show the loading overlay | ||
const useLoadingIndicator = !isLazyLoaded || apiRef.current.getRowsCount() === 0; |
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.
Maybe avoid using React hooks specific keyword use
?
const useLoadingIndicator = !isLazyLoaded || apiRef.current.getRowsCount() === 0; | |
const showLoadingIndicator = !isLazyLoaded || apiRef.current.getRowsCount() === 0; |
Or maybe an inline condition would be more readable?
if (!isLoading && (!isLazyLoaded || apiRef.current.getRowsCount() === 0)) {
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.
Small suggestion for the error handling demo to use a Snackbar instead of positioning the alert over the grid. What do you think?
function ErrorAlert({ onClick }: { onClick: () => void }) { | ||
return ( | ||
<Alert | ||
sx={{ | ||
position: 'absolute', | ||
bottom: '0', | ||
paddingX: 2, | ||
paddingY: 1, | ||
width: '100%', | ||
zIndex: 10, | ||
}} | ||
severity="error" | ||
action={ | ||
<Button color="inherit" size="small" onClick={onClick}> | ||
Retry | ||
</Button> | ||
} | ||
> | ||
Could not fetch the data | ||
</Alert> | ||
); | ||
} |
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.
function ErrorAlert({ onClick }: { onClick: () => void }) { | |
return ( | |
<Alert | |
sx={{ | |
position: 'absolute', | |
bottom: '0', | |
paddingX: 2, | |
paddingY: 1, | |
width: '100%', | |
zIndex: 10, | |
}} | |
severity="error" | |
action={ | |
<Button color="inherit" size="small" onClick={onClick}> | |
Retry | |
</Button> | |
} | |
> | |
Could not fetch the data | |
</Alert> | |
); | |
} | |
function ErrorSnackbar(props: SnackbarProps & { onRetry: () => void }) { | |
const { onRetry, ...rest } = props; | |
return ( | |
<Snackbar {...rest}> | |
<Alert | |
severity="error" | |
variant="filled" | |
sx={{ width: '100%' }} | |
action={ | |
<Button color="inherit" size="small" onClick={onRetry}> | |
Retry | |
</Button> | |
} | |
> | |
Failed to fetch row data | |
</Alert> | |
</Snackbar> | |
); | |
} |
<ErrorAlert | ||
onClick={() => { | ||
apiRef.current.unstable_dataSource.fetchRows( | ||
GRID_ROOT_GROUP_ID, | ||
retryParams, | ||
); | ||
setRetryParams(null); | ||
}} | ||
/> | ||
)} |
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.
<ErrorAlert | |
onClick={() => { | |
apiRef.current.unstable_dataSource.fetchRows( | |
GRID_ROOT_GROUP_ID, | |
retryParams, | |
); | |
setRetryParams(null); | |
}} | |
/> | |
)} | |
<ErrorSnackbar | |
open={!!retryParams} | |
onRetry={() => { | |
apiRef.current.unstable_dataSource.fetchRows( | |
GRID_ROOT_GROUP_ID, | |
retryParams, | |
); | |
setRetryParams(null); | |
}} | |
/> |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Part of #8179
Resolves #10857
Resolves #10858
Preview: https://deploy-preview-13878--material-ui-x.netlify.app/x/react-data-grid/server-side-data/lazy-loading/
Action items in progress:
Make initial end index dependent on the viewportUse page size for the initial data loadInclude [data grid] TanStack Query integration demos #14227(will be handled separately)Changelog