[core] Adopt eslint-plugin-react-compiler

mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.

https://mui.com/material-ui/

MIT License

94.03k stars 32.3k forks source link

[core] Adopt eslint-plugin-react-compiler #42548

Open aarongarciah opened 5 months ago

aarongarciah commented 5 months ago

Context

The React Compiler optimizes code that follows the rules of React. In order to make it easier to write React compliant code, the React team released an ESLint plugin that reports broken React rules: eslint-plugin-react-compiler.

The compiler requires React 19, but the ESLint plugin can be adopted earlier to be prepared.

Motivation

Follow the rules of React in our code so it can be optimized by the React compiler in the future.
Follow React best practices in all of our demos that our consumers use.

Note: at the time of writing the ESLint is experimental but it has proven to be useful for the Toolpad team.

Tasks

[x] Configure the ESLint plugin. Only activate locally and manually while it's experimental (https://github.com/mui/material-ui/pull/42555)
[x] Disable the ESLint plugin for Base (#42563)
[ ] #42564

Search keywords: react compiler, eslint

oliviertassinari commented 2 weeks ago

A larger issue on this: #44336.

aarongarciah commented 2 weeks ago

The second biggest question is: does this even make sense? I suspect that we need to cherry pick the optimizations that help and opt-out of the ones that are harmful (extra bundle size, wasted CPU cycles, extra memory), using some sort of a test app. Because we are almost exclusively leave components, maybe this could be mostly harmful, not having much in the render tree that is pruned.

@oliviertassinari we need to try it first and see how it goes. Agree on spending time first on Base UI. I wouldn't spend time today adopting the compiler in Material UI v6.

Adopting the ESLint plugin is very interesting since it surfaces many potential issues preventing us from adopting the compiler. Even if we don't adopt the compiler, the ESLint plugin seems very helpful for writing code "the React way".

oliviertassinari commented 2 weeks ago

the ESLint plugin seems very helpful for writing code "the React way".

@aarongarciah Right, ok, for fixed like #42559, this makes sense, this logic was borderline buggy. Changes like #42619, looks borderline for performance, I think we didn't clone the theme on purpose back then, to avoid overhead.

aarongarciah commented 2 weeks ago

@oliviertassinari truth is the code in example you linked is mutating props, the ESLint plugin is surfacing a real "issue" that ideally should never happen in React. Regarding performance, is it really that bad? That code is inside React.useMemo and it'd only run when the theme changes.

wilhelmlofsten commented 1 week ago

Hello! Have put up a PR for one of the issues in #42564 (my pr #44348), where if it gets accepted, there will only be one issue left (buttonBase.js) where me and another MUI contribitutor will try to solve in the coming weeks and do the last PR! So will try to achieve #42564 with eslint issue parts 😃