mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.28k stars 32.12k forks source link

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

Open oliviertassinari opened 1 month ago

oliviertassinari commented 1 month ago

Steps to reproduce

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

Steps:

  1. Take the example in https://github.com/mui/material-ui/pull/43022#discussion_r1687208774 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"
Janpot commented 1 month ago

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 commented 1 month ago

@Janpot The counter-argument is:

https://github.com/mui/material-ui/blob/b33fca964942703262eb1f7e4c0e99e3237b6fd1/packages/mui-styles/src/styled/styled.js#L125-L130

https://github.com/mui/material-ui/blob/b33fca964942703262eb1f7e4c0e99e3237b6fd1/packages/mui-base/src/Popper/Popper.tsx#L350-L397

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 commented 21 hours ago

Oh, interesting:

SCR-20240913-bgjd

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

I could see us having a small custom fork.