[DataGrid] Events firing multiple times and also while table initially renders

dheeraj1447 commented 3 years ago

[x] The issue is present in the latest release.
[x] 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:

Reload the template, you can see onPageChange event firing thrice.
Go on to the next page, event fires twice.
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 ```

DanailH commented 3 years ago

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

oliviertassinari commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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

missbruni commented 2 years ago

@dtassone can we expect the same fix for onSortModelChange ? This is also firing on mount. https://github.com/mui-org/material-ui-x/pull/1034

m4theushw commented 2 years ago

@helissonms @missbruni You can subscribe to https://github.com/mui-org/material-ui-x/issues/2635 for the onSortModelChange fix.

mui / mui-x