[mui-lab] Fix issues reported by react-compiler in mui-lab

[sai6855]

part of #42564

implementation inspired from

material-ui/packages/mui-lab/src/CalendarPicker/CalendarPicker.tsx

Lines 7 to 20 in 987bb03

	const warn = () => {
	if (!warnedOnce) {
	console.warn(
	[
	'MUI: The CalendarPicker component was moved from `@mui/lab` to `@mui/x-date-pickers`.',
	'',
	"You should use `import { CalendarPicker } from '@mui/x-date-pickers'`",
	"or `import { CalendarPicker } from '@mui/x-date-pickers/CalendarPicker'`",
	'',
	'More information about this migration on our blog: https://mui.com/blog/lab-date-pickers-to-mui-x/.',
	].join('\n'),
	);
	warnedOnce = true;

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 7c71fb6

[aarongarciah]

Which was the error being reported by eslint-plugin-react-compiler?

[sai6855]

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

[aarongarciah]

Looks good.

Unrelated to this PR: I wonder if we should change the pattern and use useEffect for this kind of warnings cc @DiegoAndai

[sai6855]

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 useEffect runs twice that in turn makes warning to show twice

[aarongarciah]

not sure this would work, as in strict mode useEffect runs twice that in turn makes warning to show twice

Not in prod environments, so it should be fine.

[DiegoAndai]

I wonder if we should change the pattern and use useEffect for this kind of warnings cc @DiegoAndai

@aarongarciah what would be the benefit?

[aarongarciah]

@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.

[DiegoAndai]

Makes sense 🙌🏼
Does React have any guidelines on how to implement warnings? How are they doing it?

[aarongarciah]

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:

• https://react.dev/learn/synchronizing-with-effects#sending-analytics
• https://react.dev/reference/rules/components-and-hooks-must-be-pure#how-to-tell-if-code-runs-in-render

[oliviertassinari]

mui/mui-x#13911, by moving to use this warnOnce helper, we should be able to solve this. It looks like we solved the problem here by making it harder for react-compiler to spot the mutation. I guess we could have used "use no memo" too.

[aarongarciah]

It looks like we solved the problem here by making it harder for react-compiler to spot the mutation

Yeah, feels hacky. I think these kind of warnings should be fired in an effect. This discussion wouldn't exist.

I guess we could have used "use no memo" too.

I'm wary of "use no memo" because it's not meant to be a long-term solution: see reactwg/react-compiler#7.

[oliviertassinari]

I think these kind of warnings should be fired in an effect

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:

• proptypes
• render
• effect

I'm wary of "use no memo" because it's not meant to be a long-term solution: see reactwg/react-compiler#7.

Right, so if we look at what great looks like:

• In this instance, we shouldn't even have to deal with the problem: [lab] Remove deprecated lab components #43025
• For other components, we should move the warnOnce() helper upstream, it shouldn't be in each MUI X components, but in Base UI [core] Remove warning message in production mui-x#13911.

[aarongarciah]

Effect don't fire on the server

True 🤦 My reasoning was that we're triggering side-effects on render, which goes against the rules of React.

proptypes

propTypes are pretty much dead (see proptypes), so we should probably focus on keep triggering warnings on render.

it shouldn't be in each MUI X components, but in Base UI

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.

[oliviertassinari]

side-effects on render

@aarongarciah I agree, but one with no negative unintended consequences, so I think it's OK.

are pretty much dead (see proptypes), so we should probably focus on keep triggering warnings on render.

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.

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.

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.

[aarongarciah]

I agree, but one with no negative unintended consequences, so I think it's OK.

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).

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.

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.

[oliviertassinari]

As I see it, ideally Base UI should export stuff that's useful for the end consumers, not 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 /utils and /internals. In the case of warnOnce() it's more likely to be a /internals because even though a third-party libraries building extension of Base UI likely needs it, it could be better to have those people duplicate the helper, so

1. we can move with more velocity, not having to care about this failing point
2. libraries we own have a nicer more integrated experience, artificially making it a bit worse for third parties 😇. (It won't be used by regular developers, not like a useEnhancedLayout would be)

In more details: https://www.notion.so/engineering-mui-utils-purpose-9a9fc9da3a004864b6c4e1f4d1f24f95