Skip to content
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] Implement Lazy loading #5214

Merged
merged 91 commits into from
Sep 1, 2022

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Jun 15, 2022

#1247

Preview -> https://deploy-preview-5214--material-ui-x.netlify.app/x/react-data-grid/row-updates/#lazy-loading

How does it work?

The approach is fairly straightforward. The new virtualization does make solving this problem more easily. The solution still doesn't on the scrollTop. It reacts when there are changes of therenderContext and emits an event that is handled by the useGridLazyLoader feature hook.
There is a small logic that finds the section between 2 given indexes of the rows array and only passes down to the user the start and end index of the rows that hadn't been loaded yet (skeleton rows).

The more interesting part is how the skeleton rows are being rendered. There is a pre-processor that adds logic to the hydrateRows. When the lazy load feature is enabled the pre-processor runs and it checks if there is a difference between the number of provided rows and the rowCount set by the user. If there is a difference then "empty" (skeleton rows, with only auto-generated id and no model added to the state). There is a small logic in the GridRow.tsx that handles the case where a row has no model and renders skeleton cells instead of GridCell.tsx component.

Last but not least, there is a new API method in the useGridRows that replaces a set of rows between 2 given indexes with a user-provided set. This is how the new rows are "loaded".

ToDo:

  • add tests
  • polush the docs
  • make the feature exprimental

@DanailH DanailH added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request plan: Pro Impact at least one Pro user v5.x labels Jun 15, 2022
@DanailH DanailH self-assigned this Jun 15, 2022
@mui-bot
Copy link

mui-bot commented Jun 15, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 334 643 453.2 473.76 122.396
Sort 100k rows ms 469 886.6 750.1 726.26 143.54
Select 100k rows ms 223.4 470.2 249.9 283.7 94.05
Deselect 100k rows ms 122 230.1 199.2 194 38.54

Generated by 🚫 dangerJS against 5a7905e

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 20, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 6, 2022
@DanailH DanailH added the feature: Rendering layout Related to the data grid Rendering engine label Jul 7, 2022
@DanailH DanailH marked this pull request as ready for review July 7, 2022 07:39
docs/data/data-grid/rows/rows.md Outdated Show resolved Hide resolved
You can find out more information about how to do that on the [server-side filter page](/components/data-grid/filtering/#server-side-filter) and on the [server-side sorting page](/components/data-grid/sorting/#server-side-sorting).
:::

{{"demo": "LazyLoadingGrid.js", "bg": "inline", "disableAd": true}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading this section and the demo, I'm wondering how often the onFetchRows is called and should I handle by myself to do not call the server if I already have all the rows

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all the rows are loaded the onFetchRows won't be called.
One optimization that is needed by the user is to implement throttle because now this callback is being called a lot while scrolling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to fetch for each row that becomes visible in the viewport. In practice, it's probably not usable in production in the current shape as it would flood the server API.

DanailH and others added 8 commits August 24, 2022 16:40
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
….test.tsx

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 24, 2022

I have pushed a simplified version of the demo, aiming to be as close as possible to how developers might fetch the data server-side. It fixes the double fetch flickers. However, it seems that we have a couple of issues. A summary of what I think is off, by order of importance (to complement the existing open comments on lines of code):

  1. When using the filter in the demo, there are a couple of strange behaviors: flickering of row rendering, crashes of out-of-range row updates, and crashes of duplicated React keys. I don't know why.
  2. The onFetchRows API itself has no notion of cancellable requests. It assumes that calls are resolved instantly. But if you fetch, sort, and request a previous sortModel resolves, it breaks your data. So IMHO, this can't be used in production by a developer. The issue can be seen when filtering the demo. The results displayed are the ones from an incomplete search because they resolve after the query will the full request. AG Grid has this notion https://ag-grid.com/javascript-data-grid/server-side-model-datasource/#success-callback. I guess it could be handled by the demo, maybe we should turn to RxJs.
  3. onFetchRows is called every time a new row is in the viewport, so when you scroll, you can easily throw 50 requests to your backend. So IMHO, this can't be used in production by a developer, he would DDoS his backend. I think that we should either scope down the feature, for example, call it something like onRowsViewportChange with a non-primary goal of data fetching or we should batch the calls by introducing a notion of chunk size so it can be used for data fetching. But maybe support of this in the demo might be enough, as an illustration. It will be painful for developers but it's still a step forward. AG Grid has this notion: https://ag-grid.com/javascript-data-grid/server-side-model-api-reference/#reference-serverSideRowModel-cacheBlockSize
  4. For the skeleton, add a small delay for appearing, like 100ms, to avoid flashes

I get now the amount of complexity there is in something like https://ag-grid.com/javascript-data-grid/server-side-model/, why it's not MIT.

@DanailH
Copy link
Member Author

DanailH commented Aug 25, 2022

Ok, I've pushed the solution a bit more and cleaned most of the issues.

  1. When using the filter in the demo, there are a couple of strange behaviors: flickering of row rendering crashes of out-of-range row updates, and crashes of duplicated React keys. I don't know why.

Should be fixed now

  1. onFetchRows is called every time a new row is in the viewport, so when you scroll, you can easily throw 50 requests to your backend.

I left it like this intentionally. We can add a line in the doc that advises the developers to implement either throttle or debounce themselves. Adding this functionality as part of the feature increases the complexity quite a lot.

  1. For the skeleton, add a small delay for appearing, like 100ms, to avoid flashes

I guess this will be added once the Core adds that prop?

  1. The onFetchRows API itself has no notion of cancellable requests. It assumes that calls are resolved instantly. But if you fetch, sort, and request a previous sortModel resolves, it breaks your data.

Can't this be solved the same way 3. is?

We can merge this version and see what the feedback will be. We can continue to polish it more but it's been quite a long time and I feel we are operating based on assumptions. Not that it is wrong but I would love to start a clean PR after we get some real-world use cases/bugs 😅

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 25, 2022

Should be fixed now

@DanailH Great, thanks for looking into it! I have found a few more issues on https://deploy-preview-5214--material-ui-x.netlify.app/x/react-data-grid/row-updates/#lazy-loading.

a. When sorting, the rows updates twice:

Screen.Recording.2022-08-25.at.17.57.57.mov

b. When opening the filtering panel with an active sort, the rows change:

Screen.Recording.2022-08-25.at.17.58.55.mov

c. When filtering with an active sort, there are rows that shouldn't match

Screen.Recording.2022-08-25.at.18.02.30.mov

And I think that it's :). It might be an issue with the demo, and not the source.

Can't this be solved the same way 3. is?

I left it like this intentionally. We can add a line in the doc that advises the developers to implement either throttle or debounce themselves. Adding this functionality as part of the feature increases the complexity quite a lot.

It's definitely more complex, but I also think why we should care. It's precisely for the hard problems that developers will come to use our stuff.

My main concern is: Are we shipping a demo that can't be used in production and with APIs that are not unstable_ so that we can't change without waiting for v6?

From what I understand, if a developer uses https://deploy-preview-5214--material-ui-x.netlify.app/x/react-data-grid/row-updates/#lazy-loading as is, he will DDoS his backend and will show rows to his end-users that don't match what they filter for (out-of-sync). So I would expect that he will need to do further work to solve 2. and 3 on his own. Since we know it, I think that we should fix it before we tell them to use the demo. I'm afraid that if we don't, we might harm the trust that developers have in their ability to copy & paste from the demo in the docs. Since the problem is complex, 💯 to only fix it in the demo. It will give us time to find an API that can then accommodate tree data, pivoting, etc.

I guess this will be added once the Core adds that prop?

We could wait for mui/material-ui#34057. However, I think that it would be great to first dogfood this idea inside MUI X, both to find an implementation that works well, and to prove that it brings value, and that it deserves to be in the core. I think that it could be a follow-up PR 👍 . It's kind of detail, a lot less important than the previous points.


Edit: It seems that this other demo https://mui.com/x/react-data-grid/row-updates/#infinite-loading is also not usable in production as is. The filtering and sorting are not working and the demo is not this straightforward to convert into network API calls. I would be in favor of working on this, considering AG Grid demos seem directly usable.

@DanailH
Copy link
Member Author

DanailH commented Aug 31, 2022

Alright, I added the last changes:

  • add the unstable_ prefix for the replaceRows API
  • add a debounce to the onFetchRows callback
  • add a warning in the docs to highlight that it's best to implement a debounce so you limit the number of requests since sends to their BE.

Regarding the problems with the demo - I guess it's a limitation with what we have in terms of the fake server. It needs additional polishing but that can be done later on in a clean PR. I will merge this PR as it states it is stable in terms of the lazy loading feature and the replace rows API.

Thank you @oliviertassinari for the detailed review and feedback.

@DanailH DanailH merged commit bd6c15a into mui:master Sep 1, 2022
@LukasTy LukasTy mentioned this pull request Sep 1, 2022
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Rendering layout Related to the data grid Rendering engine new feature New feature or request plan: Pro Impact at least one Pro user v5.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants