[material-ui] Improve getChildRef()

[sai6855]

check #42818 (comment) for more context

[mui-bot]

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 0f10b63

[aarongarciah]

Left some comments, but let's wait for @oliviertassinari since he suggested the improvements.

[oliviertassinari]

I'm missing something. Do we have a concrete example where this warning helps and is not harming (creating confusion)?

[sai6855]

You are right, console.error doesn't provide any help if child is invalid react element. Throwing error instead of logging error seems to improve DX.

Code used to get below errors

 <Slide direction="up" in={checked} mountOnEnter unmountOnExit>
        <p>sample</p>
        <p>sample</p>
  </Slide>

when just console.error is used

When thowing error instead of console.error

[oliviertassinari]

@sai6855 I think this example

<Slide direction="up" in={checked} mountOnEnter unmountOnExit>
  <p>sample</p>
  <p>sample</p>
</Slide>

supports the previous argument I was trying to develop in #42818 (comment): we increase the bundle size for end-users, so we degrade the UX. But on the opposite side, the error is still not actionable, so we don't improve the DX.
I conclude that it makes things worse.

Previously, the experience was (without TypeScript):

which feels equal.

So this didff looks like a step forward to me. Simpler, less bundle size, faster runtime:

diff --git a/packages/mui-material/src/utils/getChildRef.js b/packages/mui-material/src/utils/getChildRef.js
index a1d0c2aec5..c24cd926d3 100644
--- a/packages/mui-material/src/utils/getChildRef.js
+++ b/packages/mui-material/src/utils/getChildRef.js
@@ -1,9 +1,4 @@
-import * as React from 'react';
-
 export default function getChildRef(child) {
-  if (!child || !React.isValidElement(child)) {
-    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 child.props.propertyIsEnumerable('ref') ? child.props.ref : child.ref;

Now, if we want to avoid the confusion for developers, to not have an error thrown, but only a console error message, I could see us doing. This feels like a better DX:

diff --git a/packages/mui-material/src/Slide/Slide.js b/packages/mui-material/src/Slide/Slide.js
index d669e811d6..5b3a7a71ed 100644
--- a/packages/mui-material/src/Slide/Slide.js
+++ b/packages/mui-material/src/Slide/Slide.js
@@ -119,6 +119,10 @@ const Slide = React.forwardRef(function Slide(props, ref) {
     ...other
   } = props;

+  // Invalid children provided, bailout
+  if (!React.isValidElement(children)) {
+    return children;
+  }
+
   const childrenRef = React.useRef(null);
   const handleRef = useForkRef(getChildRef(children), childrenRef, ref);

diff --git a/packages/mui-material/src/utils/getChildRef.js b/packages/mui-material/src/utils/getChildRef.js
index a1d0c2aec5..c24cd926d3 100644
--- a/packages/mui-material/src/utils/getChildRef.js
+++ b/packages/mui-material/src/utils/getChildRef.js
@@ -1,9 +1,4 @@
-import * as React from 'react';
-
 export default function getChildRef(child) {
-  if (!child || !React.isValidElement(child)) {
-    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 child.props.propertyIsEnumerable('ref') ? child.props.ref : child.ref;

---

Now, there will also be the React 19 prop-types feedback problem for people now using types or using any:

There, I think that we simply need to create new utils, inspired by facebook/react#28328, and do:

diff --git a/packages/mui-material/src/Slide/Slide.js b/packages/mui-material/src/Slide/Slide.js
index d669e811d6..f316556579 100644
--- a/packages/mui-material/src/Slide/Slide.js
+++ b/packages/mui-material/src/Slide/Slide.js
@@ -1,6 +1,8 @@
 'use client';
 import * as React from 'react';
 import PropTypes from 'prop-types';
+import getDisplayName from '@mui/utils/getDisplayName';
+import checkPropTypes from 'prop-types/checkPropTypes';
 import { Transition } from 'react-transition-group';
 import chainPropTypes from '@mui/utils/chainPropTypes';
 import HTMLElementType from '@mui/utils/HTMLElementType';
@@ -82,11 +84,20 @@ export function setTranslateValue(direction, node, containerProp) {
   }
 }

+function muiCheckPropTypes(Component, props,) {
+  if (process.env.NODE_ENV === 'production') {
+    return;
+  }
+  return checkPropTypes(Component.propTypes, props, 'prop', getDisplayName(Component));
+}
+
 /**
  * The Slide transition is used by the [Drawer](/material-ui/react-drawer/) component.
  * It uses [react-transition-group](https://github.com/reactjs/react-transition-group) internally.
  */
-const Slide = React.forwardRef(function Slide(props, ref) {
+const Slide = React.forwardRef(function SlideIn(props, ref) {
+  muiCheckPropTypes(Slide, props);
+

cc @mui/code-infra

[Janpot]

This feels like a better DX:

I second that.

There, I think that we simply need to create new utils

Sure, I perhaps would consider moving the condition around muiCheckPropTypes(Slide, props); to shave off a few more bytes and prevent an unnecessary empty function call.

+const Slide = React.forwardRef(function SlideIn(props, ref) {
+  if (process.env.NODE_ENV !== 'production') {
+    muiCheckPropTypes(Slide, props);
+  }
+

Do we have a CI check that verifies certain utilities don't make it into production bundles?

[oliviertassinari]

I perhaps would consider moving the condition around muiCheckPropTypes(Slide, props); to shave off a few more bytes and prevent an unnecessary empty function call.

Fair enough.

It's also missing a React.version check to not warn twice in React 18 and lower.

[oliviertassinari]

Opportunity moved to #43138

[oliviertassinari]

@sai6855 The changes make sense to me now. We are back to a local maximum 👍.

If we want to get to the next local maximum, I would see:

diff --git a/packages/mui-material/src/Slide/Slide.js b/packages/mui-material/src/Slide/Slide.js
index d669e811d6..5b3a7a71ed 100644
--- a/packages/mui-material/src/Slide/Slide.js
+++ b/packages/mui-material/src/Slide/Slide.js
@@ -119,6 +119,10 @@ const Slide = React.forwardRef(function Slide(props, ref) {
     ...other
   } = props;

+  // Invalid children provided, bailout
+  if (!React.isValidElement(children)) {
+    return children;
+  }
+
   const childrenRef = React.useRef(null);
   const handleRef = useForkRef(getChildRef(children), childrenRef, ref);

combined with #43138, but better to be iterative.

[aarongarciah]

@sai6855 as part of the React 19 effort, @DiegoAndai recently moved getChildRef to mui-utils under a new name: getReactNodeRef. Can you rebase the PR and apply the potential changes there?

[sai6855]

I'm unsure if type if element should be React.ReactNode or React.ReactElement, currently this util is used by Portal and portal supports multiple nodes as children

material-ui/packages/mui-base/src/Portal/Portal.tsx

Line 5 in 0137be7

import getReactNodeRef from '@mui/utils/getReactNodeRef';

But if type of element remains as React.ReactNode we need to type case element to any to make element.props.propertyIsEnumerable('ref') work.

Where as if element type is React.ReactElement, type casting to any is not necessary as element.props won't result in type errors

[aarongarciah]

Taking into account that:

• The recommended way to use this helper seems to check with React.isValidElement first (as per [material-ui] Improve getChildRef() #43022 (comment))
• getReactNodeRef's behaviour goes like this:
  • returns the ref if the param is an element with a ref
  • returns null for elements without a ref and fragments
  • returns undefined for arrays.
  • throws an error when passing "non-object" children types (null, undefined, booleans, string, etc.)

Maybe it should be named getReactElementRef instead of getReactNodeRef? And update the param type to be React.ReactElement instead of React.ReactNode.

Here's a playground showcasing the differences between getReactElementRef and getReactNodeRef. Note how the former forces consumers to check with React.isValidElement before using it, which I think it's a better DX and avoids unexpected errors.

https://codesandbox.io/p/sandbox/getreactnoderef-zjcv2t?file=/src/App.tsx:73,21

Let me know if this makes sense or if I missed something.

cc @DiegoAndai since you worked on this helper, too.

[DiegoAndai]

The logic behind leaving the isValidElement check inside the util is that we were doing the check on most places in which we used the util so it seemed better to incorporate it into it (#43132 (comment)). I'm not against moving the check out, but I would like to understand what's the benefit of doing so. How is forcing consumers to check isValidElement better DX?

[oliviertassinari]

How is forcing consumers to check isValidElement better DX?

@DiegoAndai I tried to demonstrate it in #43022 (comment). Most of the time, there is already a warning in place to flag when using the component API wrong. Silencing the error up in the the stack (not at getReactNodeRef) makes it easy for developers to clearly notice the problem and how to fix it.

if type if element should be React.ReactNode or React.ReactElement

+1 for React.ReactElement

[DiegoAndai]

Ok, I'm on board with @oliviertassinari and @aarongarciah's proposed changes, lets go with the approach in #43022 (comment) @sai6855.

[oliviertassinari]

Sounds great 👍.

Before we move forward, I think to sync with mui/base-ui#605 (comment) so we move in the same direction.

[sai6855]

Summary of changes done in this PR

1. Removed !element || !React.isValidElement(element) check here, as it was decided it's better DX to move error up to component level here
2. Added tests for getReactElementRef https://github.com/mui/material-ui/pull/43022/files#diff-55a56a78280026badefee15f47b8823274fe80e1ca3f4a8426c3ffb53d57c1a1 and https://github.com/mui/material-ui/pull/43022/files#diff-54bc148947b0c3f0a3e6872123e20b9615bf2940c1189ce27124597a5233746d
3. Renamed getReactNodeRef to getReactElementRef
4. Updated usage of getReactElementRef in Portal component here and here