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

[react 19] checkPropTypes, ensure runtime warning continuity #43138

Open
oliviertassinari opened this issue Aug 1, 2024 · 3 comments
Open

[react 19] checkPropTypes, ensure runtime warning continuity #43138

oliviertassinari opened this issue Aug 1, 2024 · 3 comments
Labels
bug 🐛 Something doesn't work dx Related to developers' experience enhancement This is not a bug, nor a new feature React 19 support PRs required to support React 19 regression A bug, but worse

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 1, 2024

Steps to reproduce

Link to live example: https://codesandbox.io/s/frosty-snowflake-zjl49r?file=/src/Demo.js

Steps:

  1. Take the example in [core] Improve getReactElementRef() utils #43022 (comment) with React 19
<Slide direction="up" in={checked} mountOnEnter unmountOnExit>
  <p>sample</p>
  <p>sample</p>
</Slide>
  1. Toggle the switch

  2. Check the errors

SCR-20240801-kkbh

but nothing tells you how to fix it.

Current behavior

No information on what is wrong.

You could argue that TypeScript will let you know ahead of time, but what if you get the type wrong? At least, from this issue, we can collect upvotes from developers who faced the same challenge.

Expected behavior

A clear error message.

Context

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;
+  }
+  if (React.version >= '19') {
+    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) {
+  if (process.env.NODE_ENV !== 'production') {
+    muiCheckPropTypes(Slide, props);
+  }
+

Your environment

    "@emotion/react": "11.13.0",
    "@emotion/styled": "11.13.0",
    "@mui/material": "5.16.6",
    "react": "19.0.0-rc.0",
    "react-dom": "19.0.0-rc.0"
@oliviertassinari oliviertassinari added status: waiting for maintainer These issues haven't been looked at yet by a maintainer React 19 support PRs required to support React 19 labels Aug 1, 2024
@oliviertassinari oliviertassinari added the dx Related to developers' experience label Aug 1, 2024
@Janpot
Copy link
Member

Janpot commented Aug 1, 2024

Related: We could update our javascript codesandboxes to check types. Nothing about using javascript prevents us from type checking. Even just adding // @ts-check on top of that file already makes the warning appear: https://codesandbox.io/p/sandbox/youthful-lalande-dk6h7m?file=%2Fsrc%2FDemo.js

Screenshot 2024-08-01 at 12 11 11

This improves the DX by a landslide IMO. We can likely just add a tsconfig.json to our JS sandboxes to accomodate for this.


Possible simple solution with a new helper:

Perhaps we can automate this in the prop types generation script?

@oliviertassinari oliviertassinari added the waiting for 👍 Waiting for upvotes label Aug 1, 2024
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 1, 2024

@Janpot The counter-argument is:

clone: chainPropTypes(PropTypes.bool, (props) => {
if (props.clone && props.component) {
return new Error('You can not use the clone and component prop at the same time.');
}
return null;
}),

anchorEl: chainPropTypes(
PropTypes.oneOfType([HTMLElementType, PropTypes.object, PropTypes.func]),
(props) => {
if (props.open) {
const resolvedAnchorEl = resolveAnchorEl(props.anchorEl);
if (
resolvedAnchorEl &&
isHTMLElement(resolvedAnchorEl) &&
resolvedAnchorEl.nodeType === 1
) {
const box = resolvedAnchorEl.getBoundingClientRect();
if (
process.env.NODE_ENV !== 'test' &&
box.top === 0 &&
box.left === 0 &&
box.right === 0 &&
box.bottom === 0
) {
return new Error(
[
'MUI: The `anchorEl` prop provided to the component is invalid.',
'The anchor element should be part of the document layout.',
"Make sure the element is present in the document or that it's not display none.",
].join('\n'),
);
}
} else if (
!resolvedAnchorEl ||
typeof resolvedAnchorEl.getBoundingClientRect !== 'function' ||
(isVirtualElement(resolvedAnchorEl) &&
resolvedAnchorEl.contextElement != null &&
resolvedAnchorEl.contextElement.nodeType !== 1)
) {
return new Error(
[
'MUI: The `anchorEl` prop provided to the component is invalid.',
'It should be an HTML element instance or a virtualElement ',
'(https://popper.js.org/docs/v2/virtual-elements/).',
].join('\n'),
);
}
}
return null;
},
),

we won't get them with TypeScript, and yet we still need those errors. I'm convinced we need those ones. I would expect without those errors, we will see a flow of support questions, and because as support engineers, we want to have the least amount of work possible, we will actively prioritize adding those checks back for React 19.

@atomiks for example for https://github.com/mui/base-ui/blob/fa9f3fa71320d40a409d0318229ec6521fe52e54/packages/mui-base/src/Popover/Positioner/usePopoverPositioner.types.ts#L15, is there a error in case developers provide a DOM node that isn't mounted into the DOM anymore? This used to be a recurring annoying (for me, a support engineer) support question on Material UI. I'm curious to see if developers will complain about this again.


Now, do we need all the warnings, I mean for all the props? Yeah, I doubt it. I have added the "waiting for upvotes"s flag, it will help us gather data on this.

@oliviertassinari oliviertassinari changed the title [react 19] checkPropTypes [react 19] checkPropTypes, ensure runtime warning continuity Aug 2, 2024
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 12, 2024

Oh, interesting:

SCR-20240913-bgjd

https://github.com/facebook/prop-types

We might need to create a simpler version of prop-types to solve those DX issues.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work regression A bug, but worse enhancement This is not a bug, nor a new feature and removed waiting for 👍 Waiting for upvotes status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work dx Related to developers' experience enhancement This is not a bug, nor a new feature React 19 support PRs required to support React 19 regression A bug, but worse
Projects
None yet
Development

No branches or pull requests

3 participants