-
Notifications
You must be signed in to change notification settings - Fork 4.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
Try: eslint-plugin-react-compiler
#61788
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: 0 B Total Size: 1.76 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 54f002a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9173492325
|
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? |
No, the linked bug report is about function useExampleRef() {
const ref = useRef();
return ref;
}
function Example1() {
const ref = useExampleRef();
useEffect( () => {
ref.current = 'value';
} );
}
// above /react/issues/29196 vs what linter flags in this PR.
function Example2() {
const ref = useRef();
useEffect( () => {
ref.current = 'value';
} );
} |
This happens because the React Compiler ESLint plugin doesn't recognize that this is React's If you wish to try it out, try changing:
to:
and you'll see the error will be gone. That being said, I still think it's more or less the same issue because for React Compiler it's all the same - it's a different hook from its perspective, as it can't understand that this is the same React Let me know if that makes sense. |
Oh, I didn’t though about that 😅 |
This is a good reason to reconsider if we want to continue using |
6bd3449
to
f1ae3bb
Compare
Thanks for keeping it up to date, @Mamaduka. Bummer to see the |
What?
This PR experiments with introducing the
eslint-plugin-react-compiler
ESLint plugin to the repo.The plugin is still experimental, so don't merge this just yet.
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
Observe the new ESLint errors.
Testing Instructions for Keyboard
None
Screenshots or screencast
None