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

Allow the length of the dependency array in hooks to change. #20850

Closed
wants to merge 1 commit into from

Conversation

lbfalvy
Copy link

@lbfalvy lbfalvy commented Feb 19, 2021

Summary

Allow for hooks to depend on the content of variable-length arrays.
This is currently impossible since both the number of hooks and the number of dependencies per hook are fixed numbers.
The only case where dependency array comparison can still have misleading results is [...myArray, ...myOtherArray], so I also added a warning about this. This may be superfluous as the warning appears in a lot of what would now become perfectly valid situations.
resolves #18229

Test Plan

As shown in the edited test case, useEffect(() => etc, myArray) now yields expected results.

@sizebot
Copy link

sizebot commented Feb 19, 2021

Comparing: 8af27ae...47cc182

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.min.js = 122.34 kB 122.35 kB +0.01% 39.40 kB 39.41 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 128.92 kB 128.93 kB = 41.46 kB 41.46 kB
facebook-www/ReactDOM-prod.classic.js = 404.81 kB 404.82 kB = 75.19 kB 75.19 kB
facebook-www/ReactDOM-prod.modern.js = 393.16 kB 393.17 kB = 73.28 kB 73.28 kB
facebook-www/ReactDOMForked-prod.classic.js = 404.82 kB 404.83 kB = 75.19 kB 75.19 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOMServer-prod.modern.js +0.40% 47.51 kB 47.70 kB +0.10% 11.07 kB 11.08 kB
facebook-www/ReactDOMServer-prod.classic.js +0.39% 48.40 kB 48.59 kB +0.27% 11.28 kB 11.31 kB

Generated by 🚫 dangerJS against 47cc182

@lbfalvy lbfalvy force-pushed the master branch 2 times, most recently from a2a8e49 to d6e11ef Compare February 20, 2021 10:30
@lbfalvy
Copy link
Author

lbfalvy commented Feb 24, 2021

After a bit of testing I think the warning is more annoying than useful, however I can't use log levels below warn. I have a silent version at /lbfalvy/react/tree/nowarn.

Allow length of dependencies array to change.
@stale
Copy link

stale bot commented Jun 11, 2021

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.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jun 11, 2021
@HansBrende
Copy link

Bump: this is very useful functionality that would solve a lot of pain points I've encountered.

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Aug 26, 2021
@lbfalvy
Copy link
Author

lbfalvy commented Oct 9, 2021

This is very old now and I don't have the patience to fix it, and it can cause very subtle bugs as described in the issue, but I made a special hook for variable length arrays here @lbfalvy/react-utils which somewhat imporves on the situation by making the variable length opt-in.

@stale
Copy link

stale bot commented Jan 8, 2022

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.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 8, 2022
@sebmarkbage sebmarkbage deleted the branch facebook:master October 20, 2022 20:41
@gaearon
Copy link
Collaborator

gaearon commented Oct 20, 2022

Apologies for closing. We've accidentally pushed a master branch to the repo (due to an old fork), then deleted it, and this lead to a bunch of PRs getting auto-closed. GitHub seems confused and unfortunately does not offer a way to reopen this against main.

That said, I don't think we will be adding support for this. (Thanks for PR though)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Resolution: Stale Automatically closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Allow dependency length to change in hooks
6 participants