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] Fix sortModel and filterModel resetting when columns change #9239

Merged
merged 6 commits into from
Jun 9, 2023

Conversation

alexgonch
Copy link
Contributor

@alexgonch alexgonch commented Jun 5, 2023

Fixes #9204

This PR fixes the problem raised in the #9204 issue by implementing the suggestion of @cherniavskii

By replacing useEffect with useLayoutEffect, the setSortModel and setFilterModel methods are now called before the handleColumnsChange runs. This ensures that when both the columns and the respective model(s) change simultaneously, the DataGrid won't fire the onSortModelChange and onFilterModelChange events with erroneously empty models as their parameters.

This change fixes the problems I've had in my project, but I haven't tested it beyond that.

@mui-bot
Copy link

mui-bot commented Jun 5, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9239--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 605.4 933 860.2 789.44 129.456
Sort 100k rows ms 512.8 1,009.2 730.4 777.48 160.751
Select 100k rows ms 223.3 301.5 282.2 265.52 32.846
Deselect 100k rows ms 158.2 321.1 183.1 205.04 59.403

Generated by 🚫 dangerJS against 4f14f13

@cherniavskii cherniavskii added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature feature: Sorting Related to the data grid Sorting feature labels Jun 6, 2023
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Thanks!
I also added unit tests for both filtering and sorting 👍

@cherniavskii cherniavskii requested a review from MBilalShafi June 6, 2023 10:12
setProps({
columns: [{ field: 'id' }],
filterModel: {
items: [{ field: 'id', operator: 'equals', value: '1' }],
Copy link
Member

Choose a reason for hiding this comment

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

As the filterModel prop is updated, technically it's changed, shouldn't this emit a filterModelChange here? Or we only emit it once it's internally changed. (Same with sortModel)

Copy link
Member

Choose a reason for hiding this comment

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

Good question!
In controlled mode, we don't call onChange handler if the new value is the same as the prop:

hasPropChanged: newSubState !== controlState.propModel,

if (controlState.propOnChange && hasPropChanged) {
const details =
props.signature === GridSignature.DataGridPro
? { api: apiRef.current, reason }
: { reason };
controlState.propOnChange(model, details);
}

Copy link

@jonnylink jonnylink Jul 10, 2023

Choose a reason for hiding this comment

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

Just a guess, but could this by we are still experiencing an issue? Refresh once and the default filters/sorts are missing. Refresh again and they are back.

@cherniavskii cherniavskii changed the title [DataGrid] Fix sortModel and filterModel resetting when columns change [DataGrid] Fix sortModel and filterModel resetting when columns change Jun 9, 2023
@cherniavskii cherniavskii merged commit 0b13a24 into mui:master Jun 9, 2023
flaviendelangle added a commit to flaviendelangle/mui-x that referenced this pull request Jun 9, 2023
@jonnylink
Copy link

jonnylink commented Jul 10, 2023

I am sorry to report that this only partially resolved the issue for us.

  1. First load the default sort and filters are applied
  2. On refresh they are not
  3. On second refresh they are applied again
  4. (rinse repeat)

@cherniavskii
Copy link
Member

Hey @jonnylink
Could you please open a new issue with a minimal reproduction example?
Thank you!

@jonnylink
Copy link

@cherniavskii
Sure thing. I'll do that as soon as I have some time. Under a few tight deadlines. Our setup might not be super easy to reproduce (we use an IndexedDB persistent TanStack cache).

Seeing as this bug fix got us halfway there, I'm fairly confident it's related.

@fabianboerner
Copy link

this is not fixed in version 5?

@cherniavskii
Copy link
Member

@fabianboerner No, the latest stable version is v6 and we're going to release the v7 very soon.
We encourage you to migrate to v6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature feature: Sorting Related to the data grid Sorting feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamically updating DataGrid columns fires erroneous sortModelChange event with an empty model
7 participants