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

Closed
2 tasks done
dheeraj1447 opened this issue Jan 6, 2021 · 7 comments · Fixed by #838
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!
Milestone

Comments

@dheeraj1447
Copy link

dheeraj1447 commented Jan 6, 2021

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

  • Current behaviour introduced a bug that fires onPageChange event multiple times
  • Even when the XGrid renders the event fires thrice (one client event and twice server)

Expected Behavior 🤔

  • Not to fire events on table render
  • Event should fire once on its use case

Steps to Reproduce 🕹

DataGrid: https://codesandbox.io/s/material-demo-forked-9jzwr?file=/package.json
XGrid: https://codesandbox.io/s/material-demo-forked-8dylq?file=/demo.tsx

Steps:

  1. Reload the template, you can see onPageChange event firing thrice.
  2. Go on to the next page, event fires twice.
  3. Go back to the previous page, same issue.

Context 🔦

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

Your Environment 🌎

`npx @material-ui/envinfo`
 System:
    OS: Linux 5.4 Ubuntu 20.04.1 LTS (Focal Fossa)
  Binaries:
    Node: 12.20.1 - /usr/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 6.14.10 - /usr/bin/npm
  Browsers:
    Chrome: 87.0.4280.88
    Firefox: Not Found
  npmPackages:
    @emotion/styled:  10.0.27 
    @material-ui/core: 4.11.0 => 4.11.0 
    @material-ui/icons: 4.9.1 => 4.9.1 
    @material-ui/lab: 4.0.0-alpha.56 => 4.0.0-alpha.56 
    @material-ui/pickers:  3.2.10 
    @material-ui/styles:  4.11.1 
    @material-ui/system:  4.9.14 
    @material-ui/types:  5.1.0 
    @material-ui/utils:  4.10.2 
    @material-ui/x-grid: 4.0.0-alpha.14 => 4.0.0-alpha.14 
    @material-ui/x-license:  4.0.0-alpha.14 
    @types/react: 16.9.2 => 16.9.2 
    react: 16.9.0 => 16.9.0 
    react-dom: 16.9.0 => 16.9.0 
    typescript: 4.0.2 => 4.0.2 

@dheeraj1447 dheeraj1447 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 6, 2021
@DanailH DanailH added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Jan 7, 2021
@DanailH
Copy link
Member

DanailH commented Jan 7, 2021

Hi @dheeraj1447 thanks for reporting this, it is indeed an issue. We will take a look at it.

@oliviertassinari oliviertassinari removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 7, 2021
@oliviertassinari oliviertassinari changed the title [XGrid] Events firing multiple times and also while table initially renders [DataGrid] Events firing multiple times and also while table initially renders Jan 7, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 7, 2021

I have updated the reproduction to use DataGrid, focusing on the MIT version should be enough as it reproduces the same bugs as XGrid.

Regarding the expected behavior, I would go even further than @dheeraj1447 has suggested. onPageChange fires 3 times. Intuitively, I would expect it to fire 0 times. The active page never changes, it's 1. I don't think that pageCount, pageSize, paginationMode, rowCount should be returned in the callback, we are only interested in page.

I would also encourage to change the signature of the callback. I would expect onPageChange: (event, page) for consistency with the other components.

@DanailH
Copy link
Member

DanailH commented Jan 7, 2021

Regarding the expected behavior, I would go even further than @dheeraj1447 has suggested. onPageChange fires 3 times. Intuitively, I would expect it to fire 0 times. The active page never changes, it's 1.

I agree, the reason why it fires 3 times is not that the page changes but because the pagination state is being updated (items per page, total pages etc.).

@dheeraj1447
Copy link
Author

dheeraj1447 commented Jan 7, 2021

Thanks @oliviertassinari and @DanailH for your quick response. I also agree with @oliviertassinari as the current table uses TablePagination internally. You can expose the same event. Moreover, same thing is happening with onSortModelChange event. Since XGrid is a enterprise product, hope you guys fix this soon. Thank you!

@helissonms
Copy link

The same thing is happening with onSortModelChange event, on MIT version.

@missbruni
Copy link

@dtassone can we expect the same fix for onSortModelChange ? This is also firing on mount.
#1034

@m4theushw
Copy link
Member

@helissonms @missbruni You can subscribe to #2635 for the onSortModelChange fix.

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!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants