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

[List] Memoize context value #14934

Merged
merged 3 commits into from
Mar 18, 2019
Merged

[List] Memoize context value #14934

merged 3 commits into from
Mar 18, 2019

Conversation

mkermani144
Copy link
Contributor

@mkermani144 mkermani144 commented Mar 18, 2019

Related to #10778

Memoizing context value is another solution to https://reactjs.org/docs/context.html#caveats

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

There's currently an issue with shallow rendering + forwardRef + hooks. We need to adjust our test. I have prepared something locally which also improves this test as a whole unless you insist on doing this yourself.

The implementation is fine as-is.

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 18, 2019

Details of bundle changes.

Comparing: 02b6dab...7e18acd

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.01% 🔺 +0.02% 🔺 357,579 357,618 91,073 91,093
@material-ui/core/Paper 0.00% 0.00% 68,634 68,634 19,979 19,979
@material-ui/core/Paper.esm 0.00% 0.00% 62,366 62,366 19,060 19,060
@material-ui/core/Popper 0.00% 0.00% 30,454 30,454 10,525 10,525
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,390 17,390 5,730 5,730
@material-ui/core/useMediaQuery 0.00% 0.00% 2,471 2,471 1,042 1,042
@material-ui/lab 0.00% 0.00% 152,165 152,165 44,548 44,548
@material-ui/styles 0.00% 0.00% 53,839 53,839 15,566 15,566
@material-ui/system 0.00% 0.00% 17,143 17,143 4,522 4,522
Button 0.00% 0.00% 90,924 90,924 26,941 26,941
Modal 0.00% 0.00% 84,711 84,711 25,208 25,208
colorManipulator 0.00% 0.00% 3,234 3,234 1,301 1,301
docs.landing 0.00% 0.00% 51,843 51,843 11,349 11,349
docs.main +0.01% 🔺 +0.01% 🔺 649,999 650,044 201,578 201,596
packages/material-ui/build/umd/material-ui.production.min.js +0.01% 🔺 +0.02% 🔺 305,844 305,881 83,862 83,876

Generated by 🚫 dangerJS against 7e18acd

@eps1lon eps1lon self-requested a review March 18, 2019 08:01
@eps1lon eps1lon changed the title [List] Use useMemo for context value in order to prevent extra re-renders [List] Memoize context value Mar 18, 2019
@eps1lon eps1lon added performance component: list This is the name of the generic UI component, not the React module! labels Mar 18, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 18, 2019

@mkermani144 It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

@henrylearn2rock
Copy link

Any chance of merging to 3.9.x? Thanks

@ryancogswell
Copy link
Contributor

Any chance of merging to 3.9.x?

@henrylearn2rock Sorry, but that won't be possible since the solution requires the useMemo hook, and 3.9.x doesn't require a version of React that supports hooks.

@oliviertassinari
Copy link
Member

@henrylearn2rock v4.0.0-beta.0 is in good shape. You should be able to migrate, starting today. If you want to be more cautious, you can wait a week or two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: list This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants