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] onSortModelChange firing multiple times and also while table initially renders #2635

Closed
2 tasks done
thuatole opened this issue Sep 18, 2021 · 15 comments · Fixed by #2724
Closed
2 tasks done
Assignees
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! performance

Comments

@thuatole
Copy link

thuatole commented Sep 18, 2021

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

  • Current behaviour introduced a bug that fires onSortModelChange event multiple times

Expected behavior 🤔

No response

Steps to reproduce 🕹

Source https://codesandbox.io/s/material-demo-forked-t5tq0?file=/demo.js
Steps:

  1. Reload the template, you can see onSortModelChange event firing thrice.

Context 🔦

Due to multiple onSortModelChange event firing app is re-rendering eventually causing performance issues.

Your environment 🌎

`npx @mui/envinfo`
  System:
    OS: macOS 10.15.7
  Binaries:
    Node: 14.16.1 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.12 - /usr/local/bin/npm
  Browsers:
    Chrome: 93.0.4577.82
    Edge: Not Found
    Firefox: Not Found
    Safari: 14.0.3
  npmPackages:
    @emotion/react: ^11.4.1 => 11.4.1 
    @emotion/styled: ^11.3.0 => 11.3.0 
    @mui/core:  5.0.0-alpha.46 
    @mui/lab: ^5.0.0-alpha.46 => 5.0.0-alpha.46 
    @mui/material: ^5.0.0-rc.1 => 5.0.0-rc.1 
    @mui/private-theming:  5.0.0-rc.1 
    @mui/styled-engine:  5.0.0-rc.1 
    @mui/system:  5.0.0-rc.1 
    @mui/types:  7.0.0-rc.1 
    @mui/utils:  5.0.0-rc.1 
    @mui/x-data-grid-pro: ^4.0.0 => 4.0.0 
    @mui/x-license-pro:  4.0.0 
    @types/react: 16.14.5 => 16.14.5 
    react: 16.14.0 => 16.14.0 
    react-dom: 16.14.0 => 16.14.0 
    styled-components: 5.2.1 => 5.2.1 
    typescript: 4.2.3 => 4.2.3 

Order ID 💳 (optional)

No response

@thuatole thuatole added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 18, 2021
@DanailH DanailH added bug 🐛 Something doesn't work performance priority: low component: data grid This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 20, 2021
@trickreich
Copy link

trickreich commented Sep 28, 2021

why this is priority low?
for us the library isn't usable at all, because we have dynamic columns, then this is a infinite loop!

image

@elyesbenabdelkader
Copy link
Contributor

I agree with trickreich, this is a blocker.

@flaviendelangle
Copy link
Member

@trickreich can you provide an example that causes the inifinite loop ?

@trickreich
Copy link

I've built something similiar to our project: https://codesandbox.io/s/mui-x-data-grid-onsortmodelchange-infinite-loop-t72pm
Click on one column and you will see the infinite loop.

image

@flaviendelangle
Copy link
Member

flaviendelangle commented Sep 28, 2021

#2724 fixes the problem describes in the first message. It does not call onSortModelChange on mount or on prop change but only when you change the value through the UI or the imperative API.

I don't know if it will also fix your bug, I'll wait for the publication of the package on Codesandbox to check 👍
Try to always memoize your rows and columns props. We have behaviors triggered when those props changes so even if it works with my fix, you will have poor performances and potential future bugs.

@flaviendelangle
Copy link
Member

flaviendelangle commented Sep 28, 2021

https://codesandbox.io/s/mui-x-data-grid-onsortmodelchange-infinite-loop-forked-20qrb

@trickreich #2724 does seems to fix your bug
From what I see, the infinite loop was the following :

Your component renders with new columns => The Grid updates the sortModel to remove the potential columns not existing anymore in the current sortModel => The Grid calls onSortModelChange => Restart the loop

I added a check to avoid updating the sortModel if no sort item has been removed during the removal of potential non existing columns.

But you should also memoize the column to avoid issues.

@trickreich
Copy link

Pretty nice, thanks for the quick response and help.
Yeah, I've used useMemo before, but that's not working out of the box. (reference equality).

@karanaditya993
Copy link

@flaviendelangle any update on when #2724 will be merged? Thanks

@flaviendelangle
Copy link
Member

I will be in the next release
Not sure they will be one this week, so probably next Thursday

@elyesbenabdelkader
Copy link
Contributor

@flaviendelangle can we please have this in v4.0.2 ASAP?

@m4theushw
Copy link
Member

@elyesbenabdelkader You can use MUI X v5 while the application stays on MUI Core v4. Here's the instructions. I don't know when there will be a v4.0.2 release as the effort is now on v5.

@mauro-ni
Copy link

mauro-ni commented Oct 15, 2021

@elyesbenabdelkader in order to temporary fix the problem I'm considering using the following approach:

import _ from 'lodash';

const [sortModel, setSortModel] = useState([
     // default filters
]);

<DataGrid
    sortModel={sortModel}
    onSortModelChange={(model) => {
        if (!_.isEqual(model, sortModel)) {
            setSortModel(model);
        }
    }}
/>

@m4theushw I know that this is far to be optimal, but it seems to work. What do you think?

Many thanks.

Mauro N.

@flaviendelangle
Copy link
Member

It should work fine if you don't want to have v4 and v5 MUI core in your bundle.

@karanaditya993
Copy link

karanaditya993 commented Oct 15, 2021

@flaviendelangle do you have an estimate as to when v5 will be released? Also, this issue mentions that we can't use v5 mui-x with v4 mui core? Is this correct? Is there a way we can have this specific fix patched as a minor fix to the v4 mui-x bundle? It's a breaking change to the v4 data-grid/pro functionality.

@m4theushw
Copy link
Member

Also, this issue mentions that we can't use v5 mui-x with v4 mui core?

@karanaditya993 You can use MUI X v5 and configure MUI Core v4 to not conflict with v5. See these instructions.

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! performance
Projects
None yet
8 participants