`react-hooks/rules-of-hooks`: Add support for `do/while` loops #28714

tyxla · 2024-04-02T13:37:19Z

Summary

Currently, react-hooks/rules-of-hooks does not support do/while loops - I've also reported this in #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

facebook-github-bot · 2024-04-02T13:37:24Z

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!

react-sizebot · 2024-04-02T13:42:37Z

Comparing: ae90522...ff8ac24

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.82 kB	1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	506.86 kB	506.86 kB	=	90.71 kB	90.71 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.69 kB	6.69 kB	+0.05%	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	=	511.79 kB	511.79 kB	=	91.43 kB	91.42 kB
facebook-www/ReactDOM-prod.classic.js	=	604.26 kB	604.26 kB	=	106.92 kB	106.92 kB
facebook-www/ReactDOM-prod.modern.js	=	580.48 kB	580.48 kB	=	103.00 kB	103.01 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 0f822bf

tyxla · 2024-04-17T07:59:01Z

@eps1lon I noticed you've been working on the ESLint rules, will you have a chance to take a look? Thanks 🙌

github-actions · 2024-07-16T08:06:23Z

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 · 2024-07-23T09:04:47Z

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!

tyxla · 2024-07-23T09:27:23Z

Bump! This is still a relevant addition to the existing ESLint plugin IMHO.

tyxla · 2024-07-23T09:30:59Z

cc @mofeiZ perhaps? Sorry for the direct ping, but no one ever replied to this one, and I still think it's useful.

josephsavona · 2024-07-23T12:29:13Z

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.

vercel · 2024-07-23T12:29:17Z

@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.

tyxla · 2024-07-23T12:32:15Z

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.

josephsavona · 2024-07-23T14:04:54Z

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.

tyxla · 2024-07-23T14:10:54Z

React Compiler already correctly handles do..while and all other forms of loops when detecting conditional hooks.

That's even better, thanks for clarifying!

github-actions · 2024-10-21T15:05:11Z

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.

tyxla · 2024-10-21T15:06:29Z

Not stale IMHO.

mofeiZ

@tyxla left a few questions. I agree with @josephsavona, react compiler-based eslint does correctly detect and fix this, but we know that not everyone has upgraded yet

packages/eslint-plugin-react-hooks/src/RulesOfHooks.js

vercel · 2024-10-22T10:04:43Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 22, 2024 5:14pm

tyxla · 2024-10-22T10:18:48Z

@mofeiZ, thanks for the review and the valuable feedback!

I've addressed it all, and this PR should be ready for one last look. 👀

packages/eslint-plugin-react-hooks/src/RulesOfHooks.js

mofeiZ

thanks for the fix and new test fixtures @tyxla!

## Summary  Currently, `react-hooks/rules-of-hooks` does not support `do/while` loops - I've also reported this in #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 ``` DiffTrain build for [9daabc0](9daabc0)

Mathias-S · 2024-12-10T11:09:37Z

FYI, this change causes false positives when for/while loops are used in functional components, see #31687

Example of code that now gets (incorrectly) flagged by this rule:

const MyComponent = () => {
  const [state, setState] = useState(0);

  for (let i = 0; i < 10; i++) {
    console.log(i);
  }

  return <div></div>;
};

tyxla · 2024-12-10T13:12:32Z

@Mathias-S thanks for the report! We might need to take this a different way.

I think I might have a quick solution as I played with a few alternative solutions before. Stay tuned.

tyxla · 2024-12-10T13:28:12Z

Follow up fix in #31720, @Mathias-S please take a look when you get a chance.

The latest release of eslint-plugin-react-hooks incorrectly flags uses of hooks as occurring in a loop if a component function body contains a loop. Downgrading to version 5.0.0 is a workaround until the React team publishes a release with the fix in facebook/react#28714 (merged but not yet released). See facebook/react#31687

tyxla changed the title ~~Fix/rule of hooks do while~~ react-hooks/rules-of-hooks: Add support for do/while loops Apr 2, 2024

tyxla mentioned this pull request Apr 2, 2024

Bug: react-hooks/rules-of-hooks does not support do/while loops #28713

Closed

facebook-github-bot added the CLA Signed label Apr 2, 2024

github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Jul 16, 2024

github-actions bot closed this Jul 23, 2024

josephsavona reopened this Jul 23, 2024

github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Jul 23, 2024

github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Oct 21, 2024

github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Oct 21, 2024

mofeiZ reviewed Oct 21, 2024

View reviewed changes

packages/eslint-plugin-react-hooks/src/RulesOfHooks.js Outdated Show resolved Hide resolved

packages/eslint-plugin-react-hooks/src/RulesOfHooks.js Show resolved Hide resolved

tyxla and others added 4 commits October 22, 2024 12:31

rules-of-hooks: add tests for do/while scenarios

aacc434

rules-of-hooks: fix do/while

d065129

Add test for hook called inside a while clause

dc7a41f

Remove extra caching logic

33cb28f

tyxla force-pushed the fix/rule-of-hooks-do-while branch from 33a8918 to 33cb28f Compare October 22, 2024 10:04

vercel bot deployed to Preview October 22, 2024 10:06 View deployment

tyxla requested a review from mofeiZ October 22, 2024 10:18

mofeiZ reviewed Oct 22, 2024

View reviewed changes

packages/eslint-plugin-react-hooks/src/RulesOfHooks.js Outdated Show resolved Hide resolved

Remove extra whitespace

0f822bf

vercel bot deployed to Preview October 22, 2024 17:14 View deployment

tyxla requested a review from mofeiZ October 22, 2024 17:17

mofeiZ approved these changes Oct 22, 2024

View reviewed changes

mofeiZ merged commit 9daabc0 into facebook:main Oct 22, 2024
184 checks passed

tyxla deleted the fix/rule-of-hooks-do-while branch October 23, 2024 08:01

This was referenced Oct 23, 2024

Upgrade React from 45804af1-20241021 to 28668d39-20241023 vercel/next.js#71734

Closed

Upgrade React from 45804af1-20241021 to 28668d39-20241023 vercel/next.js#71745

Merged

This was referenced Dec 10, 2024

Bug: [eslint-plugin-react-hooks] incorrectly reports an error when hook is called outside of a loop. #31687

Closed

react-hooks/rules-of-hooks: Improve support for do/while loops #31720

Merged

tschaub mentioned this pull request Jan 8, 2025

Downgrade eslint-plugin-react-hooks planetlabs/eslint-config-planet#104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`react-hooks/rules-of-hooks`: Add support for `do/while` loops #28714

`react-hooks/rules-of-hooks`: Add support for `do/while` loops #28714

tyxla commented Apr 2, 2024

facebook-github-bot commented Apr 2, 2024

react-sizebot commented Apr 2, 2024 •

edited

Loading

tyxla commented Apr 17, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 23, 2024

tyxla commented Jul 23, 2024

tyxla commented Jul 23, 2024

josephsavona commented Jul 23, 2024

vercel bot commented Jul 23, 2024

tyxla commented Jul 23, 2024

josephsavona commented Jul 23, 2024

tyxla commented Jul 23, 2024

github-actions bot commented Oct 21, 2024

tyxla commented Oct 21, 2024

mofeiZ left a comment

vercel bot commented Oct 22, 2024 •

edited

Loading

tyxla commented Oct 22, 2024

mofeiZ left a comment

Mathias-S commented Dec 10, 2024 •

edited

Loading

tyxla commented Dec 10, 2024

tyxla commented Dec 10, 2024

react-hooks/rules-of-hooks: Add support for do/while loops #28714

react-hooks/rules-of-hooks: Add support for do/while loops #28714

Conversation

tyxla commented Apr 2, 2024

Summary

How did you test this change?

facebook-github-bot commented Apr 2, 2024

Action Required

Process

react-sizebot commented Apr 2, 2024 • edited Loading

Critical size changes

Significant size changes

tyxla commented Apr 17, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Jul 23, 2024

tyxla commented Jul 23, 2024

tyxla commented Jul 23, 2024

josephsavona commented Jul 23, 2024

vercel bot commented Jul 23, 2024

tyxla commented Jul 23, 2024

josephsavona commented Jul 23, 2024

tyxla commented Jul 23, 2024

github-actions bot commented Oct 21, 2024

tyxla commented Oct 21, 2024

mofeiZ left a comment

Choose a reason for hiding this comment

vercel bot commented Oct 22, 2024 • edited Loading

tyxla commented Oct 22, 2024

mofeiZ left a comment

Choose a reason for hiding this comment

Mathias-S commented Dec 10, 2024 • edited Loading

tyxla commented Dec 10, 2024

tyxla commented Dec 10, 2024

`react-hooks/rules-of-hooks`: Add support for `do/while` loops #28714

`react-hooks/rules-of-hooks`: Add support for `do/while` loops #28714

react-sizebot commented Apr 2, 2024 •

edited

Loading

vercel bot commented Oct 22, 2024 •

edited

Loading

Mathias-S commented Dec 10, 2024 •

edited

Loading