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] Fix Accessing element.ref #42818

Merged
merged 13 commits into from
Jul 18, 2024
3 changes: 2 additions & 1 deletion packages/mui-material/src/Fade/Fade.js
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
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
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
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
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
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
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

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

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