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

[DataGrid] Setting page with ApiRef in useEffect does not work anymore #1815

Closed
dtassone opened this issue Jun 2, 2021 · 5 comments · Fixed by #2099
Closed

[DataGrid] Setting page with ApiRef in useEffect does not work anymore #1815

dtassone opened this issue Jun 2, 2021 · 5 comments · Fixed by #2099
Assignees
Labels
bug 🐛 Something doesn't work

Comments

@dtassone
Copy link
Member

dtassone commented Jun 2, 2021

Broken story
https://material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-pagination--page-2-api

Expected to render rows 51-100 of 2000

Instead it renders rows 1-50 of 2000

TODO add as snap test

@dtassone dtassone added the bug 🐛 Something doesn't work label Jun 2, 2021
@oliviertassinari
Copy link
Member

@dtassone When you open an issue, please always follow the issue template. For instance, we miss a codesandbox and the version.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 2, 2021

Regarding the change of behavior, it originates from #1583 (not from the refactoring part, but from the fix of #1517). I should have made it more obvious by commenting on it. I thought that the tradeoff was negligible so I didn't mention it.

As a side note. I should have moved the refactoring to a different PR, this is a great example of what I did sucks.

@flaviendelangle
Copy link
Member

flaviendelangle commented Jul 1, 2021

The useEffect in the story component occurs just after the useEffect in useData is triggered.
So the story component has not yet rerendered with the data.

If I add a check on the rows length in the useEffect in the story component, the setPage occurs after the story component has rerenderer with the data

  React.useEffect(() => {
    if (data.rows.length) {
      apiRef.current.setPage(1);
    }
  }, [data, apiRef]);

But, it's still to soon because it occurs before the useEffect in useGridPagination has updated the pagination state with rowCount > 0.


For me the real issue here is that we have async cascade state update, so the useEffect in the story component can be called with an intermediate state.

We should trigger the setGridState currently in the useEffect in useGridPagination synchronously whenever the visibleRowCount changes, not in a useEffect.


By the way, these async state update create weird intermediate states.
For instance, in this story, the setGridState that updated the pagination with rowCount: 0 and pageSize: 50 is triggered after the setGridState that updates the rows with rows: [.... 2000 items].

Here is the detailed order :

  1. We render the story component with 0 rows

  2. We update the options state with the pageSize: 50 in useOptionsProp

  3. We update the row state with 0 rows in useGridRows

  4. We update the pagination state in useGridPagination with the initial state (which have 0 rows and a pageSize of 100)

  5. We render the story component with 2 000 rows

  6. We update the row state with 2 000 rows in useGridRows

  7. We update the pagination state in useGridPagination with the state of step 3. The amount of rows is still 0, but the pageSize has changed to 50 since last render so the useEffect is triggered)

  8. We update the pagination state in useGridPagination with the state of step 6. The amount of rows is 2 000, the rowCount is now correctly initialized.

Without my fix in the story, the call to setPage occurs between the step 4 and 5, so it has a consistent state (rows.length === 0 and pagination.rowCount === 0

With my fix in the story, (if(data.rows.length) apiRef.current.setPage(1)), the call to setPage occurs between the step 7 and 8, so it has in inconsistent state (rows.length === 2000 but pagination.rowCount === 0

@flaviendelangle
Copy link
Member

Relates to #2044 (check after refactoring the pagination with the new controlled state pattern)

@flaviendelangle
Copy link
Member

flaviendelangle commented Jul 22, 2021

I confirm that #2099 fixes this one. proof: https://deploy-preview-2099--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-pagination--page-2-api&globals=measureEnabled:false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants