[material-ui] Improve getReactElementRef() utils

[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 b84c85d

[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]

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
5. Updated logic to check for ref in different react versions here d05cdaf based on [material-ui] Improve getReactElementRef() utils #43022 (comment)

[aarongarciah]

Looks good.

I don't have a strong opinion on using propertyIsEnumerable vs check the React major. Although I tend to prefer to check features over versions, the version check works well, too. If someone has a strong opinion, speak now 😄

[aarongarciah]

I'll wait for one last pass from @oliviertassinari before merging since he was the proponent of this change.

[oliviertassinari]

Looks great

[aarongarciah]

Thank you @sai6855!