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

[data grid] Fix implicit dependency on react-dom #5121

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jun 5, 2022

I was on mui/material-ui#33015 (comment), having a quick look at what could explain the jump in bundle size in the data grid at version v5.9.0. We went from 78.1 to 122.1 KB. Compared to react-table 15.7 KB it can be intimidating. I could identify the origin down to #4332, we depend on react-dom:

but we don't list the dependency.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Jun 5, 2022
@@ -1,5 +1,5 @@
import * as React from 'react';
import ReactDOM from 'react-dom';
import * as ReactDOM from 'react-dom';
Copy link
Member Author

Choose a reason for hiding this comment

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

A bonus refactoring, to match the docs: https://reactjs.org/docs/react-dom.html.

@mui-bot
Copy link

mui-bot commented Jun 5, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 247.2 450.7 374.9 351.64 86.536
Sort 100k rows ms 434.5 821.5 744.8 699.34 136.5
Select 100k rows ms 129.5 194.5 178.1 165.96 26.516
Deselect 100k rows ms 95.7 277 234.2 181.9 70.847

Generated by 🚫 dangerJS against 2d9c977

@oliviertassinari oliviertassinari added dependencies Update of dependencies core Infrastructure work going on behind the scenes labels Jun 5, 2022
@oliviertassinari oliviertassinari merged commit 606dc99 into mui:master Jun 6, 2022
@oliviertassinari oliviertassinari deleted the fix-dependencies branch June 6, 2022 19:07
joserodolfofreitas pushed a commit to joserodolfofreitas/mui-x that referenced this pull request Jun 13, 2022
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 13, 2022

It worked, it's more accurate now, only 126.4 kB gzipped 😁

Screenshot 2022-06-13 at 14 41 59

https://bundlephobia.com/package/@mui/x-data-grid@5.12.0

@@ -1,5 +1,5 @@
import * as React from 'react';
import ReactDOM from 'react-dom';
import * as ReactDOM from 'react-dom';
Copy link
Member Author

Choose a reason for hiding this comment

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

@m4theushw When do we plan to use React 18? It would then be

Suggested change
import * as ReactDOM from 'react-dom';
import * as ReactDOM from 'react-dom/client';

per https://reactjs.org/blog/2022/03/29/react-v18.html#new-client-and-server-rendering-apis

Copy link
Member

Choose a reason for hiding this comment

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

I'm only waiting for the core repo to adopt React 18 in the infra. But nothing stops us from adopting earlier. #4155 is open.

alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
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! core Infrastructure work going on behind the scenes dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants