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

[material-ui] Element ref access React 19 compatibility #43132

Merged
merged 5 commits into from
Aug 1, 2024

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Jul 31, 2024

Closes: #42604
Part of: #42824

Make element ref access compatible with React 19 and older versions. This work was started in #42818, and this PR covers other cases and refactors the util to be shared between packages.

These commits were cherry picked from #42824.

@DiegoAndai DiegoAndai added package: material-ui Specific to @mui/material React 19 support PRs required to support React 19 labels Jul 31, 2024
@DiegoAndai DiegoAndai requested a review from mnajdova July 31, 2024 18:34
@DiegoAndai DiegoAndai self-assigned this Jul 31, 2024
@DiegoAndai DiegoAndai changed the title [material-ui] Element ref access React 19compatibility [material-ui] Element ref access React 19 compatibility Jul 31, 2024
@mui-bot
Copy link

mui-bot commented Jul 31, 2024

Netlify deploy preview

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

@material-ui/utils: parsed: +1.48% , gzip: +1.07%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 528f5cc

@DiegoAndai DiegoAndai force-pushed the element-ref-access-compatibility branch from 227a3e0 to d7e5c39 Compare July 31, 2024 19:20
Comment on lines 17 to 18
: // @ts-expect-error element.ref is not included in the ReactElement type
// We cannot check for it, but isValidElement is true at this point
Copy link
Member Author

@DiegoAndai DiegoAndai Jul 31, 2024

Choose a reason for hiding this comment

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

The ref property does not exist in ReactElement. We've silenced this error in our codebase since: https://github.com/mui/material-ui/pull/24565/files#diff-dcdcbff6b975b552227833f729bf59d31e97e4876a9d6ce982a07c9e1436d9bfR90

Here are other occurrences, probably copy-pasted: https://github.com/search?q=repo%3Amui%2Fmaterial-ui%20%22TODO%20upstream%20fix%22&type=code

I don't think they'll fix it now that they moved the ref to the props.

I don't know if there's a better way to handle this, I couldn't find one.

Copy link
Member

Choose a reason for hiding this comment

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

I would just recommend if you find an open issue about this, to link in the comment above so we know once it's merged that we can remove the @ts-ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find any issue in the original PR, DefinitelyTyped issues, or DefinitelyTyped discussions. I don't know if they have discussed it previously. I don't think it will get much traction as it's probably been like this forever and it's now removed in React 19.

I created a discussion asking for this and linked it.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Left few nits, the logic looks good 👌

Comment on lines 37 to 40
const handleRef = useForkRef(
React.isValidElement(children) ? getElementRef(children) : null,
forwardedRef,
);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const handleRef = useForkRef(
React.isValidElement(children) ? getElementRef(children) : null,
forwardedRef,
);
const handleRef = useForkRef(getElementRef(children), forwardedRef);

We can likely simplify, as the getElementRef handles this already.

Copy link
Member Author

Choose a reason for hiding this comment

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

I refactored to handle this case and renamed the util accordingly 😊

packages/mui-material/src/Portal/Portal.tsx Outdated Show resolved Hide resolved
Comment on lines 17 to 18
: // @ts-expect-error element.ref is not included in the ReactElement type
// We cannot check for it, but isValidElement is true at this point
Copy link
Member

Choose a reason for hiding this comment

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

I would just recommend if you find an open issue about this, to link in the comment above so we know once it's merged that we can remove the @ts-ignore.

@DiegoAndai DiegoAndai merged commit 36ad1db into mui:next Aug 1, 2024
19 checks passed
@DiegoAndai DiegoAndai deleted the element-ref-access-compatibility branch August 1, 2024 13:30
DiegoAndai added a commit to DiegoAndai/material-ui that referenced this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: material-ui Specific to @mui/material React 19 support PRs required to support React 19
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui] Accessing element.ref was removed in React 19
4 participants