Skip to content

Conversation

@KenanYusuf
Copy link
Member

@KenanYusuf KenanYusuf commented Apr 17, 2025

For the data grid, users are seeing unwanted re-renders when using theme.components.MuiDataGrid.defaultProps mui/mui-x#17128. It seems to be a result of themedProps here not being stable across renders.

This PR wraps the result of getThemeProps in a React.useMemo to make the returned value of useThemeProps stable across renders.


I'm conscious this change affects every Material UI component, so if that's undesirable, perhaps in the data grid package we could bypass the hook and call getThemeProps directly, memoizing the returned value there.

@KenanYusuf KenanYusuf added the scope: system The system, the design tokens / styling foundations used across components. eg. @mui/system with MUI label Apr 17, 2025
@mui-bot
Copy link

mui-bot commented Apr 17, 2025

Netlify deploy preview

https://deploy-preview-45943--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against bccee29

theme = theme[themeId] || theme;
}
return getThemeProps({ theme, name, props });
return React.useMemo(() => getThemeProps({ theme, name, props }), [theme, name, props]);
Copy link
Member

@siriwatknp siriwatknp Apr 18, 2025

Choose a reason for hiding this comment

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

I doubt that props will always be a new object on every render?
see this sandbox https://codesandbox.io/p/sandbox/react-new

React.useMemo would not help.

Copy link
Member Author

@KenanYusuf KenanYusuf Apr 22, 2025

Choose a reason for hiding this comment

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

React.useMemo would not help.

Memoizing the return value of getThemeProps does fix this specific issue we're seeing, see mui/mui-x#17490

More context here: #43120 (comment)

@romgrk
Copy link
Contributor

romgrk commented Apr 18, 2025

I haven't read the details of this PR, just commenting to point out you should also read #43120 for more context. I think I didn't even fix this problem in that PR, I just ended doing some refactoring.

edit: Yeah I didn't fix the problem following this discussion. I think I wanted to fix it in some other way but I forgot how.

@KenanYusuf
Copy link
Member Author

Closing this PR in favour of mui/mui-x#17490

@KenanYusuf KenanYusuf closed this Apr 23, 2025
@KenanYusuf KenanYusuf deleted the memoize-use-theme-props branch April 23, 2025 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance scope: system The system, the design tokens / styling foundations used across components. eg. @mui/system with MUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants