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] Replace forwardRef with a shim for forward compatibility #15955

Merged
merged 9 commits into from
Dec 23, 2024

Conversation

lauri865
Copy link
Contributor

Fixes #15770

The shim asserts that Ref is always part of the render function props (even if that is not the case in older versions), this is to force typescript to warn us in case we're accidentally spreading over the ref. Refactored everything to have ref as the last prop as a result:
Screenshot 2024-12-20 at 10 47 52

Ref warning doesn't work with slotProps though currently, as they take a slice of the props without the ref. But that can be improved as well I suppose to avoid any footguns.

@mui-bot
Copy link

mui-bot commented Dec 20, 2024

Deploy preview: https://deploy-preview-15955--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 6c788e6

@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Dec 20, 2024
@lauri865
Copy link
Contributor Author

lauri865 commented Dec 20, 2024

I'm not exactly sure why docs:api started failing, but the culprit is GridToolbarDensitySelector component.

@arminmeh arminmeh requested a review from romgrk December 20, 2024 19:26
@arminmeh arminmeh changed the title [data grid] Replace forwardRef with a shim for forward compatibility [DataGrid] Replace forwardRef with a shim for forward compatibility Dec 20, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 20, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 20, 2024
@arminmeh
Copy link
Contributor

FYI @mui/x
You can use this in other components as well

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 29, 2024

Should we do the same in http://github.com/mui/material-ui and https://github.com/mui/base-ui to improve performance?

Or is this meant to be a temporary workaround for a regression in React 19?

@cherniavskii
Copy link
Member

Should we do the same in http://github.com/mui/material-ui and mui/base-ui to improve performance?

Yes, I think so 👍🏻

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 30, 2024

Looking a bit closer, forwardRef in React 19 looks like this: https://github.com/facebook/react/blob/7aa5dda3b3e4c2baa905a59b922ae7ec14734b24/packages/react/src/ReactForwardRef.js#L51. Almost the same as in React 18.
It seems to create a custom element type. Looking at how React resolves this type internally, I couldn't find obvious source of slowness.

So if this is accurate, we should revert this PR once facebook/react#31613 is fixed.

Edit: OK, there is also need to factor

// Footgun: <Component ref={ref} {...props} /> will break past React 19, but the types will now warn us that we should use <Component {...props} ref={ref} /> instead.

Don't we have tone of instances of this, e.g.

https://github.com/mui/material-ui/blob/9574765d827b72545a10806db1bb1f7b3d2f54b9/packages/mui-material/src/Rating/Rating.js#L530

https://github.com/mui/material-ui/blob/9574765d827b72545a10806db1bb1f7b3d2f54b9/packages/mui-material/src/ButtonBase/ButtonBase.js#L306

@oliviertassinari oliviertassinari added the React 19 support About improving React 19 support label Dec 30, 2024
@cherniavskii
Copy link
Member

So if this is accurate, we should revert this PR once facebook/react#31613 is fixed.

Chances are it won't be fixed – see #15770 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! performance React 19 support About improving React 19 support
Projects
None yet
7 participants