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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Jul 18, 2024
Merged

[material-ui] Fix Accessing element.ref #42818

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3ecc332
fix
sai6855 Jul 2, 2024
916c3a5
fix
sai6855 Jul 2, 2024
9ee9592
fix tests
sai6855 Jul 2, 2024
117305f
add getChildRef util
sai6855 Jul 3, 2024
e781158
show diego react error
sai6855 Jul 3, 2024
a8e9190
Revert "show diego react error"
sai6855 Jul 3, 2024
ec910e5
add getChildRef d.ts file
sai6855 Jul 3, 2024
1d5f95d
handle chidlren edge cases
sai6855 Jul 3, 2024
7fa5eda
fix type
sai6855 Jul 3, 2024
2f926cd
update getChildRef logic
sai6855 Jul 3, 2024
5bc21fa
rename children to child
sai6855 Jul 7, 2024
916818e
remove checking child count
sai6855 Jul 7, 2024
95a319b
fix comment
sai6855 Jul 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/mui-material/src/Fade/Fade.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import elementAcceptingRef from '@mui/utils/elementAcceptingRef';
import { useTheme } from '../zero-styled';
import { reflow, getTransitionProps } from '../transitions/utils';
import useForkRef from '../utils/useForkRef';
import getChildRef from '../utils/getChildRef';

const styles = {
entering: {
Expand Down Expand Up @@ -48,7 +49,7 @@ const Fade = React.forwardRef(function Fade(props, ref) {

const enableStrictModeCompat = true;
const nodeRef = React.useRef(null);
const handleRef = useForkRef(nodeRef, children.ref, ref);
const handleRef = useForkRef(nodeRef, getChildRef(children), ref);

const normalizedTransitionCallback = (callback) => (maybeIsAppearing) => {
if (callback) {
Expand Down
3 changes: 2 additions & 1 deletion packages/mui-material/src/Grow/Grow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { Transition } from 'react-transition-group';
import { useTheme } from '../zero-styled';
import { getTransitionProps, reflow } from '../transitions/utils';
import useForkRef from '../utils/useForkRef';
import getChildRef from '../utils/getChildRef';

function getScale(value) {
return `scale(${value}, ${value ** 2})`;
Expand Down Expand Up @@ -61,7 +62,7 @@ const Grow = React.forwardRef(function Grow(props, ref) {
const theme = useTheme();

const nodeRef = React.useRef(null);
const handleRef = useForkRef(nodeRef, children.ref, ref);
const handleRef = useForkRef(nodeRef, getChildRef(children), ref);

const normalizedTransitionCallback = (callback) => (maybeIsAppearing) => {
if (callback) {
Expand Down
3 changes: 2 additions & 1 deletion packages/mui-material/src/Slide/Slide.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import useForkRef from '../utils/useForkRef';
import { useTheme } from '../zero-styled';
import { reflow, getTransitionProps } from '../transitions/utils';
import { ownerWindow } from '../utils';
import getChildRef from '../utils/getChildRef';

// Translate the node so it can't be seen on the screen.
// Later, we're going to translate the node back to its original location with `none`.
Expand Down Expand Up @@ -119,7 +120,7 @@ const Slide = React.forwardRef(function Slide(props, ref) {
} = props;

const childrenRef = React.useRef(null);
const handleRef = useForkRef(children.ref, childrenRef, ref);
const handleRef = useForkRef(getChildRef(children), childrenRef, ref);

const normalizedTransitionCallback = (callback) => (isAppearing) => {
if (callback) {
Expand Down
3 changes: 2 additions & 1 deletion packages/mui-material/src/Tooltip/Tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import useForkRef from '../utils/useForkRef';
import useId from '../utils/useId';
import useControlled from '../utils/useControlled';
import tooltipClasses, { getTooltipUtilityClass } from './tooltipClasses';
import getChildRef from '../utils/getChildRef';

function round(value) {
return Math.round(value * 1e5) / 1e5;
Expand Down Expand Up @@ -545,7 +546,7 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) {
};
}, [handleClose, open]);

const handleRef = useForkRef(children.ref, setChildNode, ref);
const handleRef = useForkRef(getChildRef(children), setChildNode, ref);

// There is no point in displaying an empty tooltip.
// So we exclude all falsy values, except 0, which is valid.
Expand Down
3 changes: 2 additions & 1 deletion packages/mui-material/src/Zoom/Zoom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import elementAcceptingRef from '@mui/utils/elementAcceptingRef';
import { useTheme } from '../zero-styled';
import { reflow, getTransitionProps } from '../transitions/utils';
import useForkRef from '../utils/useForkRef';
import getChildRef from '../utils/getChildRef';

const styles = {
entering: {
Expand Down Expand Up @@ -48,7 +49,7 @@ const Zoom = React.forwardRef(function Zoom(props, ref) {
} = props;

const nodeRef = React.useRef(null);
const handleRef = useForkRef(nodeRef, children.ref, ref);
const handleRef = useForkRef(nodeRef, getChildRef(children), ref);

const normalizedTransitionCallback = (callback) => (maybeIsAppearing) => {
if (callback) {
Expand Down
1 change: 1 addition & 0 deletions packages/mui-material/src/utils/getChildRef.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default function getChildRef(children: React.ReactElement): React.Ref<any> | null;
10 changes: 10 additions & 0 deletions packages/mui-material/src/utils/getChildRef.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as React from 'react';

export default function getChildRef(children) {
aarongarciah marked this conversation as resolved.
Show resolved Hide resolved
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
if (!children || !React.isValidElement(children) || React.Children.count(children) !== 1) {
Copy link
Member

@oliviertassinari oliviertassinari Jul 5, 2024 •
edited
Loading

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.

Suggested change
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?

Suggested change
if (!children || !React.isValidElement(children) || React.Children.count(children) !== 1) {
if (!children || !React.isValidElement(children)) {

Copy link
Contributor Author

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

Copy link
Member

@oliviertassinari oliviertassinari Jul 18, 2024 •
edited
Loading

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:

Suggested change
if (!children || !React.isValidElement(children) || React.Children.count(children) !== 1) {

Copy link
Contributor Author

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;
+    }

return null;
}
// 'ref' is passed as prop in React 19, whereas 'ref' is directly attached to children in React 18
// below check is to ensure 'ref' is accessible in both cases
return children.props.propertyIsEnumerable('ref') ? children.props.ref : children.ref;
}