-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
[core] React 19 compatibility #605
Conversation
Netlify deploy preview |
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.
Connecting the dots with the other projects: Aaron went through a lot of those changes mui/material-ui#42824. I think it would be great to have his review.
) => JSX.Element; | ||
) => React.JSX.Element; |
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.
👍 to bring this to the main branch
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 intend to have everything (except for package.json changes) merged to master.
@@ -79,7 +79,7 @@ export function useComponentRenderer< | |||
|
|||
let resolvedRenderProp: | |||
| ComponentRenderFn<React.HTMLAttributes<any>, OwnerState> | |||
| React.ReactElement; | |||
| React.ReactElement<any>; |
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.
| React.ReactElement<any>; | |
| React.ReactElement<unknown>; |
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.
We don't really care about the shape of the props here, especially that it's an internal hook and its type shouldn't be publicly visible. To constrain it a bit more, I changed it to Record<string, unknown>
let childRef; | ||
if (isReactVersionAtLeast(19)) { | ||
childRef = typeof render !== 'function' ? render.props.ref : null; | ||
} else { | ||
childRef = typeof render !== 'function' ? render.ref : null; | ||
} |
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.
Better fork after the typeof
.
I'm sure this is best: mui/material-ui#42818 (comment). The explore we are exploring in mui/material-ui#43022. To decide then use the same solution through the codebase.
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.
Better fork after the typeof
You mean const childRef = typeof render !== 'function' ? isReactVersionAtLeast(19) ? render.props.ref : render.ref : null;
? This makes it a nested ternary which I believe we disallow in our ESLint rules.
I'm sure this is best: mui/material-ui#42818 (comment).
How do you measure "best"? The version check is faster than property check (https://jsbench.me/mrm16egz7n/1) and the function call explains pretty well that we're branching because something changed in React version.
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.
@michaldudak I was proposing to go from:
let childRef;
if (isReactVersionAtLeast(19)) {
childRef = typeof render !== 'function' ? render.props.ref : null;
} else {
childRef = typeof render !== 'function' ? render.ref : null;
}
to:
let childRef;
if (typeof render !== 'function') {
childRef = isReactVersionAtLeast(19) ? render.props.ref : render.ref;
} else {
childRef = null;
}
something we can later convert to:
let childRef;
if (typeof render !== 'function') {
childRef = getReactElementRef(render);
} else {
childRef = null;
}
How do you measure "best"?
To be defined 😄. We had another iteration in mui/material-ui#43022 with @sai6855. I could see the purpose of going with the React version check. I don't see one path as clearly better than the other, so happy either way.
The only ask is that we are moving to a place where Base UI is exporting one helper and Material UI, etc. using it. I don't think that this should be duplicated.
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 was proposing to go...
All right, makes sense 👍
The only ask is that we are moving to a place where Base UI is exporting one helper and Material UI, etc. using it. I don't think that this should be duplicated.
I see your point. I don't feel comfortable sharing Base UI utils just yet, though, even internally. When we're closer to the release and breaking changes are less likely, we can review the whole codebase and move utilities where they ultimately belong.
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 don't feel comfortable sharing Base UI utils just yet, though, even internally
This doesn't go in opposition with the ask 👍. We could duplicate the code of the helper until this happens (e.g. we have done this in MUI X mui/mui-x#14528 we had the same warnOnce()
helper duplicated between packages). The outcome I'm trying to go after is: learn once, fix everywhere once. If one is better than the other, there is no reason for one project to do it differently than the other. If the difference is marginal, we are much better serviced using the same approach, so we can move everything at once when we learn more about the limits.
@@ -7,7 +7,7 @@ import { styled } from '@mui/system'; | |||
export default function UnstyledTooltipFollowCursor() { | |||
return ( | |||
<div style={{ display: 'flex', gap: 12 }}> | |||
<Tooltip.Root followCursorAxis="both"> | |||
<Tooltip.Root trackCursorAxis="both"> |
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.
It's not clear to me why it started to fail now, but I guess it's a good thing it fails.
@aarongarciah, I'd appreciate it if you could take a look at it as well. |
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.
Nice! I couldn't find any potential improvement related React 19 support.
Work in progress to ensure Base UI is compatible with React 19.
After this is approved, I'll revert all the changes to package.json files, and, if the checks are green, merge it into master.