facebook
`react-hooks/rules-of-hooks`: Add support for `do/while` loops
Summary
Currently, react-hooks/rules-of-hooks does not support do/while loops - I've also reported this in https://github.com/facebook/react/issues/28713.
This PR takes a stab at adding support for do/while by following the same logic we already have for detecting while loops.
After this PR, any hooks called inside a do/while loop will be considered invalid.
We're also adding some unit tests to confirm that the behavior is working as expected.
Fixes #28713.
How did you test this change?
I've added unit tests that cover the case and verified that they pass by running:
yarn test packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js --watch
I've also verified that the rest of the tests continue to pass by running:
yarn test
and
yarn test --prod
Hi @tyxla!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
Comparing: 70484844bfd47382ad0011e0066ccf25d1a84464...e0940902ad875f6f91368d28a5e4bf85c54c69c0
Critical size changes
Includes critical production bundles, as well as any change greater than 2%:
| Name | +/- | Base | Current | +/- gzip | Base gzip | Current gzip |
|---|---|---|---|---|---|---|
| oss-stable/react-dom/cjs/react-dom.production.js | = | 6.68 kB | 6.68 kB | = | 1.83 kB | 1.82 kB |
| oss-stable/react-dom/cjs/react-dom-client.production.js | = | 499.83 kB | 499.83 kB | = | 89.64 kB | 89.64 kB |
| oss-experimental/react-dom/cjs/react-dom.production.js | = | 6.69 kB | 6.69 kB | = | 1.83 kB | 1.83 kB |
| oss-experimental/react-dom/cjs/react-dom-client.production.js | = | 504.65 kB | 504.65 kB | = | 90.35 kB | 90.35 kB |
| facebook-www/ReactDOM-prod.classic.js | = | 599.81 kB | 599.81 kB | = | 105.88 kB | 105.88 kB |
| facebook-www/ReactDOM-prod.modern.js | = | 573.85 kB | 573.85 kB | = | 101.75 kB | 101.75 kB |
Significant size changes
Includes any change greater than 0.2%:
(No significant changes)
Generated by :no_entry_sign: dangerJS against 33a8918cd67aaf01d27f32c5aa04daa7b86dc069
@eps1lon I noticed you've been working on the ESLint rules, will you have a chance to take a look? Thanks 🙌
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!
cc @mofeiZ perhaps? Sorry for the direct ping, but no one ever replied to this one, and I still think it's useful.
We need to figure out our strategy for the ESLint plugin. React Compiler also implements the conditional hook call check for all loop types, using a robust general-purpose approach based on the control flow graph. Long-term we expect to replace the existing plugin w the compiler-powered version.
But we can take a look and see if it makes sense to proceed here in the meantime. I haven’t had a chance to look at the code yet though.
@tyxla is attempting to deploy a commit to the Meta Open Source Team on Vercel.
A member of the Team first needs to authorize it.
![vercel[bot] avatar](https://avatars.githubusercontent.com/in/8329?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thank you @josephsavona 🙌
FWIW, we're also interested in leveraging the React Compiler eslint plugin (we've been experimenting with it), and I'd be happy to come up with a fix for it if that same bug is present. Sounds like a good opportunity to learn more about its codebase.
I'd be happy to come up with a fix for it if that same bug is present
I appreciate the offer! But what I meant is that React Compiler already correctly handles do..while and all other forms of loops when detecting conditional hooks. But definitely try it out and let us know if you see any issues!
In the meantime we’ll review the code here.
React Compiler already correctly handles do..while and all other forms of loops when detecting conditional hooks.
That's even better, thanks for clarifying!