-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
ESLint: Enable eslint-plugin-react-compiler
#61788
Conversation
Size Change: 0 B Total Size: 1.82 MB ℹ️ View Unchanged
|
It will trigger an ESLint error everywhere where we ignore a
Seems like it will try hard to let us follow all the rules, which can create quite some additional work 😐 |
I kinda like the new stricter rules, but I agree it would take some time to bring the codebase into shape. P.S. I'll push fixes for some low-hanging errors separately. |
I'd love to help with those too. Please ping me for reviews! |
@tyxla, should we start a list of false positive errors that linter generates and then report those upstream? Example
|
Yep, definitely. The top-level await could also be one to report (currently not occurring because I've ignored the files where it happens). |
9a2d9a4
to
5921e1e
Compare
Flaky tests detected in 3bfdcd2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11907672938
|
I noticed that new lint rules do not like the hook pattern used by the command API: Example: useCommandLoader( {
name: 'core/edit-site/reset-global-styles',
hook: useGlobalStylesResetCommands,
} ); |
The solution to that will most likely be just renaming them to not follow the hook naming convention. After all, they are technically not hooks since they're violating the rules of hooks and React can't recognize them as hooks through static code analysis:
|
54f002a
to
6bd3449
Compare
It looks like, after the latest update, the plugin complains about setting the |
Yes, looks like it's a known bug: facebook/react#29196 |
I'm talking about different case that's is supported in React and ref is coming from local I'm not able to reproduce it on React Compiler Playground though. Screenshot |
Perhaps I'm misunderstanding, but isn't that precisely the bug reported in the issue I linked above? |
87dba34
to
bd1fea1
Compare
@tyxla, what do you think about merging the new link rules? Are there any possible downsides? |
Co-authored-by: Pascal Birchler <pascalb@google.com>
13e17fe
to
3bfdcd2
Compare
eslint-plugin-react-compiler
eslint-plugin-react-compiler
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@Mamaduka, this one is ready for review - please take a look when you have a chance.
Sorry @Mamaduka, which ones do you mean? In any case, I'd like to land this one first, and then we can tinker with any customizations in subsequent PRs. In the meantime, in 3bfdcd2 I've moved the ESLint config to the project level instead of the package since having a beta plugin in the package is debatable and could create some maintenance issues. |
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.
Sorry @Mamaduka, which ones do you mean? In any case, I'd like to land this one first, and then we can tinker with any customizations in subsequent PRs.
This is mostly a general question, in case you've spotted something on the React repo that warrants delaying a merge.
The ESLint plugin seems stable enough for merging ✅
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.
I don't have enough insights on wether version 19.0.0-beta-8a03594-20241020 is stable enough. The code changes itself look great. Excellent work on enabling all these rules inside Gutenberg repository.
In the meantime, in 3bfdcd2 I've moved the ESLint config to the project level instead of the package since having a beta plugin in the package is debatable and could create some maintenance issues.
Makes perfect sense. There is also this challenge that the ESLint plugin targets React 19. It doesn't seem to be a problem here, but you never know with wider set of projects.
Thank you @Mamaduka and @gziolo 🙌
There's a newer beta, and there's one every few days. I'm planning to keep the plugin up to date in subsequent PRs, but I want to enable it so folks stop introducing violations.
Yes, once React 19 actually gets released, this will be a bigger issue to consider. |
Bumping to the latest version here: #67106 |
What?
This PR introduces the
eslint-plugin-react-compiler
ESLint plugin to the repo.Even if the plugin is still beta, it will help us to have it in the repo in order to maintain a friendlier environment for React 19 and the React Compiler.
We're experimenting with adopting React Compiler through the Babel plugin in #66361.
Why?
It's recommended that this ESLint plugin is used to help improve the quality of your codebase.
How?
The
eslint-plugin-react-compiler
plugin can be used without the React Compiler itself. We're installing it and enabling it. Also ignoring a few files that include top-levelawait
which seems to break the linter right now.Testing Instructions
Verify there are no ESLint errors and the static analysis check passes.
Testing Instructions for Keyboard
None
Screenshots or screencast
None