react-hooks/rules-of-hooks: Improve support for do/while loops
#31720
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| const [state, setState] = useState(0); | ||
| for (let i = 0; i < 10; i++) { |
There was a problem hiding this comment.
Note that the hook is called in a valid way, outside of the loop.
| function isInsideDoWhileLoop(node) { | ||
| while (node) { | ||
| if ( node.type === 'DoWhileStatement' ) { | ||
| return true; | ||
| } | ||
| node = node.parent; | ||
| } | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Adding an extra utility to traverse up the tree to check if the node is called within a do/while loop.
There was a problem hiding this comment.
yesterday was the first time i looked at the code in this plugin. i'm curious why all the "is hook inside loop" logic isn't handled this way. seems like the same could be done for all types of loops/cycles and invalid code. i found the "counting segments" code somewhat hard to follow and this function is much more clear to me. maybe the counting segments was done for performance reasons?
without fully understanding the existing code, it seems like this plugin could be rewritten so that we first find all "hook" functions, then call a "isInsideCycle" that climbs up the tree looking for nodes like DoWhileStatement, IfStatement, or ForStatement etc.
anyways, good work on this (and thanks)!
There was a problem hiding this comment.
I agree - it does feel quite complex at first glance, and my initial expectation was that these would be handled top-down, with loops and control structures at the top, and then function/hook calls at the bottom, but I assumed there was a good reason (likely performance) for it to be that way. Anyway, rewriting that would be a major overhaul of the plugin, and one definitely outside of this PR's scope.
| @@ -295,7 +305,7 @@ export default { | |||
| if (pathList.has(segment.id)) { | |||
| const pathArray = Array.from(pathList); | |||
| const cyclicSegments = pathArray.slice( | |||
| pathArray.indexOf(segment.id) - 1, | |||
| pathArray.indexOf(segment.id) + 1, | |||
| @@ -485,7 +495,7 @@ export default { | |||
| for (const hook of reactHooks) { | |||
| // Report an error if a hook may be called more then once. | |||
| // `use(...)` can be called in loops. | |||
| if (cycled && !isUseIdentifier(hook)) { | |||
| if ( (cycled || isInsideDoWhileLoop(hook)) && !isUseIdentifier(hook)) { | |||
There was a problem hiding this comment.
We're now considering a hook "cycled" also if it's called within a do/while loop.
| @@ -520,7 +530,8 @@ export default { | |||
| if ( | |||
| !cycled && | |||
| pathsFromStartToEnd !== allPathsFromStartToEnd && | |||
| !isUseIdentifier(hook) // `use(...)` can be called conditionally. | |||
| !isUseIdentifier(hook) && // `use(...)` can be called conditionally. | |||
| !isInsideDoWhileLoop(hook) // wrapping do/while loops are checked separately. | |||
There was a problem hiding this comment.
Specifically excluding hooks inside do/while calls because we're handling those cases separately in the above check.
tyxla
force-pushed
the
fix/do-while-again
branch
from
9932384 to
cbb5ecd
Compare
|
@mofeiZ or @josephsavona - could I please ask for a review of this fix? Thanks in advance! |
…ebook#31720) DiffTrain build for [7c4a7c9](facebook@7c4a7c9)
Summary
This is an alternative to #28714.
Reverts the main fix there and implements a different solution by adding special handling for
DoWhileStatement.Also adds a unit test that was breaking the previous fix (props to @skratchdot for it).
Please, refer to the inline comments for more information.
Fixes #31687. Also see #28714 (comment).
How did you test this change?
I've added unit tests that cover the case and verified that they pass by running:
I've also verified that the rest of the tests continue to pass by running:
and