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

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

Open
2 of 3 tasks
aarongarciah opened this issue Jun 6, 2024 · 5 comments
Open
2 of 3 tasks

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

aarongarciah opened this issue Jun 6, 2024 · 5 comments
Assignees
Labels
core Infrastructure work going on behind the scenes performance scope: code-infra Specific to the core-infra product

Comments

@aarongarciah
Copy link
Member

aarongarciah commented Jun 6, 2024

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

Search keywords: react compiler, eslint

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 6, 2024

A larger issue on this: #44336.

@oliviertassinari oliviertassinari added React 19 support About improving React 19 support performance and removed React 19 support About improving React 19 support labels Nov 6, 2024
@aarongarciah
Copy link
Member Author

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
Copy link
Member

oliviertassinari commented Nov 8, 2024

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
Copy link
Member Author

@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
Copy link
Contributor

wilhelmlofsten commented Nov 14, 2024

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 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes performance scope: code-infra Specific to the core-infra product
Projects
None yet
Development

No branches or pull requests

3 participants