-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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] Fix Accessing element.ref #42818
Conversation
Netlify deploy previewhttps://deploy-preview-42818--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
We're going to encounter this issue in more components (at the very least https://github.com/search?q=repo:mui/material-ui%20children.ref&type=code). I wonder if:
Thoughts @DiegoAndai? |
Thanks for working on this @sai6855. I agree with @aarongarciah that we should use this issue and PR to cover all cases of the |
Updated issue, @sai6855 please update this PR's title when you have time to do so 😊 |
This comment was marked as outdated.
This comment was marked as outdated.
This reverts commit e781158.
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.
Looks good to me. I'll let @DiegoAndai take a last look before merging.
import * as React from 'react'; | ||
|
||
export default function getChildRef(children) { | ||
if (!children || !React.isValidElement(children) || React.Children.count(children) !== 1) { |
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.
This feels wrong. The prop type is already here to validate this constraint, but without bundle size or runtime overhead in production.
if (!children || !React.isValidElement(children) || React.Children.count(children) !== 1) { |
I could see value if it means a clearer failure mode, it gives us a single clear console log or exception thrown, but not sure it's what we do here.
As for React 19 not supporting prop types anymore, I still pretty convinced that we need to bring in an equivalent support where TypeScript is unable to cover the same constraints.
At the very least, the last segment is unreachable, no?
if (!children || !React.isValidElement(children) || React.Children.count(children) !== 1) { | |
if (!children || !React.isValidElement(children)) { |
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.
fair, since components using getChildRef
handling this, we might not need checking count here. removed -> 916818e
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.
fair, since components using getChildRef handling this, we might not need checking count here.
@sai6855 I don't think it's about where getChildRef
is called.
If React.isValidElement(children)
is false, then it return null.
If React.isValidElement(children)
is true, then it check React.Children.count(children)
but it will always return 1 since it's children is a valid element.
So || React.Children.count(children) !== 1
always wastes CPU/network work, so it should be removed.
Now, on the use of || !React.isValidElement()
, I think that it really depends on the DX we want to provide.
If we want to keep the same DX tradeoff as before, we should remove it, it slows the runtime with nothing in exchange.
If we want to make it so that even if developers can render a component without a children, get a console.error, type error not no exception that developers could be deceived by, then we could either decide to keep it, or probably better, to add a if (process.env.NODE_ENV !== 'production' && React.isValidElement(children)) { return null }
in the render body of the function. In which case we should also remove it.
So I conclude that great looks like this:
if (!children || !React.isValidElement(children) || React.Children.count(children) !== 1) { |
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.
thanks for detailed explaintation, opened PR to refactor -> #43022
TL,DR; of PR is
- if (!child || !React.isValidElement(child)) {
- return null;
+ if (process.env.NODE_ENV !== 'production') {
+ if (!React.isValidElement(child)) {
+ console.error(
+ [
+ "MUI: getChildRef doesn't accept array as a child.",
+ 'Consider providing a single element instead.',
+ ].join('\n'),
+ );
+ return 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.
LGTM
@aarongarciah should we merge this one? |
closes #42604
PS: tried to create a working sandbox but i guess sandboxes linked to PR commit is not working