-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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
Fix Accessing element.ref was removed in React 19 #74334
base: canary
Are you sure you want to change the base?
Fix Accessing element.ref was removed in React 19 #74334
Conversation
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
element: React.ReactElement | ||
): React.Ref<any> | undefined { | ||
// 'ref' is passed as prop in React 19, whereas 'ref' is directly attached to children in older versions | ||
if (parseInt(React.version, 10) >= 19) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure how you guys want to check this, another option is:
if (parseInt(React.version, 10) >= 19) { | |
if ((element.props as any).propertyIsEnumerable('ref')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.prototype.propertyIsEnumerable.call(element.props, 'ref')
is preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, let's just use the one Radix-UI uses. That one is tested already: https://github.com/radix-ui/primitives/blob/dae8ef4920b45f736e2574abf23676efab103645/packages/react/presence/src/Presence.tsx#L173-L190
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections. Maybe not ideal for bundle size though? radix-ui/primitives#2934 (comment).
Material-UI used: https://github.com/mui/material-ui/pull/43132/files#diff-2aabece439eee5bb9309cc46b1b3c3b690ee8a7231cbac1ba495cf832e506152R16 until it changed to copy Base UI to reduce bundle size.
Base UI uses: https://github.com/mui/base-ui/blob/a14ea0e7600e99a9c052978ee916e3348638b796/packages/react/src/utils/getReactElementRef.ts#L12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing for React.version is not robust. Feature detection is always preferable. And in that regard the one in Radix UI is actually battle tested. We can optimize later for implementations that have been tested against the full support matrix.
Seems private. Gives me a 404. |
Oops, fixed. |
See https://codesandbox.io/p/devbox/muddy-http-h3r423?file=%2Fpages%2Findex.tsx%3A12%2C5&workspaceId=ws_JTzKEPRDQXMzzhKd3x4aSM as a reproduction of the problem
Context #72873 (comment) doesn't look accurate. However, going to https://github.com/mui/material-ui/tree/master/examples/material-ui-nextjs-pages-router and opening the /about page is enough to reproduce the problem.