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

🐛 useHookAtTopLevel doesn't flag hook usages after an early return #696

Closed
1 task done
arendjr opened this issue Nov 10, 2023 · 3 comments · Fixed by #1018
Closed
1 task done

🐛 useHookAtTopLevel doesn't flag hook usages after an early return #696

arendjr opened this issue Nov 10, 2023 · 3 comments · Fixed by #1018
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug

Comments

@arendjr
Copy link
Contributor

arendjr commented Nov 10, 2023

Environment information

N/A, I used the playground.

What happened?

When a React component contains an early return, calls to hooks after that point are not correctly identified as being conditional.

For instance, see the following playground example: https://www.biomejs.dev/playground?lintRules=all&code=aQBtAHAAbwByAHQAIAB7ACAAdQBzAGUAUwB0AGEAdABlACAAfQAgAGYAcgBvAG0AIAAiAHIAZQBhAGMAdAAiADsACgAKAGYAdQBuAGMAdABpAG8AbgAgAE0AeQBDAG8AbQBwAG8AbgBlAG4AdAAoAHsAIABiAGEAcgAgAH0AKQAgAHsACgAgACAAaQBmACAAKAAhAGIAYQByACkAIAB7AAoAIAAgACAAIAByAGUAdAB1AHIAbgAgAG4AdQBsAGwAOwAKACAAIAB9AAoACgAgACAAYwBvAG4AcwB0ACAAWwBmAG8AbwAsACAAcwBlAHQARgBvAG8AXQAgAD0AIAB1AHMAZQBTAHQAYQB0AGUAKAAxACkAOwAKAAoAIAAgAHIAZQB0AHUAcgBuACAAPABkAGkAdgA%2BAHsAZgBvAG8AfQA8AC8AZABpAHYAPgA7AAoAfQA%3D

Expected result

The call to useState() should be flagged as conditional.

As a sidenote, I think it would be better if the useHookAtTopLevel rule were named noConditionalHookCalls instead, since that's its real purpose. As this example shows, being at the top-level isn't a guarantee something is not conditional.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@ematipico
Copy link
Member

Thank you @arendjr for reporting. We would also like a reproduction for the eslint hook rule, so we can confirm the incorrect behaviour

@ematipico ematipico added the S-Needs repro Status: needs a reproduction label Nov 10, 2023
@arendjr
Copy link
Contributor Author

arendjr commented Nov 11, 2023

I don’t know how to make an ESLint reproduction, but I can link you the source that confirms this should be flagged: https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level

Don’t call Hooks inside loops, conditions, or nested functions. Instead, always use Hooks at the top level of your React function, before any early returns.

(emphasis mine)

@arendjr
Copy link
Contributor Author

arendjr commented Nov 13, 2023

I saw how to make a reproduction in another issue, so you can use this link: https://stackblitz.com/edit/node-jfks6q?file=src%2Findex.js

❯ npm run lint

> node-starter@0.0.0 lint
> eslint src/**/*

/home/projects/node-jfks6q/src/index.js
  8:25  error  React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?  react-hooks/rules-of-hooks

✖ 1 problem (1 error, 0 warnings)

@ematipico ematipico added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug and removed S-Needs repro Status: needs a reproduction labels Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants