Update hooks-effect.md - #3534
Conversation
Warn about variable-length dependency arrays
|
Hi @lbfalvy! Thank you for your pull request and welcome to our community. Action RequiredIn 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. ProcessIn 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 If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Deploy preview for reactjs ready! Built without sensitive environment variables with commit 3b8bda5 |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| > | ||
| >We recommend using the [`exhaustive-deps`](https://github.com/facebook/react/issues/14920) rule as part of our [`eslint-plugin-react-hooks`](https://www.npmjs.com/package/eslint-plugin-react-hooks#installation) package. It warns when dependencies are specified incorrectly and suggests a fix. | ||
| > | ||
| >Also note that if you compose the dependencies array dynamically its length will not be checked, but the two arrays will be compared up to the length of the old one. When you pass variable-length arrays in second argument, always prepend the length of the array like so: `[myArray.length, ...myArray]` |
There was a problem hiding this comment.
Thanks for the work!
I would rather note that this is not supported. React will explicitly warn:
Warning: The final argument passed to useEffect changed size between renders. The order and size of this array must remain constant.
The behavior about what elements are compared is not mentioned and might just be an implementation detail.
To be sure, I would propose this suggestion as an addition to the warning if this is indeed the recommended fix. The relevant warning is issued here: https://github.com/facebook/react/blob/696e736be11b22e8d4fa40438d235b63db7fddde/packages/react-reconciler/src/ReactFiberHooks.old.js#L331-L339
If we want to recommend an "always" then the warning should include it as well. If there are cases where we don't want to include myArray.length we should describe them here.
There was a problem hiding this comment.
I checked the comparison logic in React and it appears that it only compares up to the shorter of the two lengths so while my suggestion works the explanation is factually incorrect.
In my rather limited experience with React, hooks being dependent on the indices of a variable length array is a pretty normal phenomenon and I can't think of any other way to handle them beside what I described here, so I think an "always" is warranted.
There was a problem hiding this comment.
I checked the comparison logic in React and it appears that it only compares up to the shorter of the two lengths so while my suggestion works the explanation is factually incorrect.
That's fine. But the source code of React does not imply a behavior we can rely on.
so I think an "always" is warranted.
I'm not worried about whether this is warranted. If it is, then this should be part of the warning itself. So to confirm your proposal, please propose this to the React core as well and see what the core maintainers have to say. This will get a response faster.
Fixed description of dependency comparison behavior
4 participants
Warn about variable-length dependency arrays
The question appeared under react/react#18229, where I also described other merits of this approach.