-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[mui-lab] Fix issues reported by react-compiler in mui-lab #42880
Conversation
Netlify deploy previewhttps://deploy-preview-42880--material-ui.netlify.app/ Bundle size report |
* @ignore - do not document. | ||
*/ | ||
export default React.forwardRef(function DeprecatedAlert(props, ref) { | ||
const warn = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which was the error being reported by eslint-plugin-react-compiler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect.b If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render)eslint(react-compiler/react-compiler)
reason for error is warnedOnce
is defined outside of component and changed inside of component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Unrelated to this PR: I wonder if we should change the pattern and use useEffect
for this kind of warnings cc @DiegoAndai
not sure this would work, as in strict mode |
Not in prod environments, so it should be fine. |
@aarongarciah what would be the benefit? |
@DiegoAndai be compliant with React rules i.e. do not perform side effects on render (see https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render). Also, firing the warning only when the component is mounted and not on every render. |
Makes sense 🙌🏼 |
There's no official guide on how to implement warnings afaik, but I think it should be pretty similar to logging analytics. There are a couple of examples of that: |
mui/mui-x#13911, by moving to use this |
Yeah, feels hacky. I think these kind of warnings should be fired in an effect. This discussion wouldn't exist.
I'm wary of |
I'm not following the reasoning. Effect don't fire on the server, but we need to report wrong API usage on the server too. We need to deduplicate error messages regardless. So to me, from best to worst:
Right, so if we look at what great looks like:
|
True 🤦 My reasoning was that we're triggering side-effects on render, which goes against the rules of React.
What's the reasoning to put this helper in Base UI? Feels wrong to put everything that is common under Base UI only because it's/it'll be the common denominator. |
@aarongarciah I agree, but one with no negative unintended consequences, so I think it's OK.
Yeah, it might facebook/react#28992. We have kind of created a helper to bring propTypes back in MUI X https://github.com/mui/mui-x/blob/a26c90438da07d16e7a1d534f8bf015a3f80b2df/packages/x-data-grid/src/internals/utils/propValidation.ts#L47. I think we should push deeper in this direction, so we have a standardized solution to this DX problem.
The thought process: Base UI needs this helper. Material UI is an extension of Base UI, Material UI needs this helper too. Why should we have this helper duplicated: end-users bundle load it twice, two reset functions to call in tests. |
The only problem I see is the React Compiler not optimizing some components because of the render time warnings (if the compiler gets smarter to detect how we warn).
As I see it, ideally Base UI should export stuff that's useful for the end consumers, not only to ourselves. If these utils are meant for "internal" use only, I think it would be better to have a package dedicated to share utils and such. |
@aarongarciah Ah yes, it's what I separate between
In more details: https://www.notion.so/engineering-mui-utils-purpose-9a9fc9da3a004864b6c4e1f4d1f24f95 |
part of #42564
implementation inspired from
material-ui/packages/mui-lab/src/CalendarPicker/CalendarPicker.tsx
Lines 7 to 20 in 987bb03