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

Fire useEffect in reverse component depth order #3354

Merged
merged 3 commits into from
Dec 7, 2021

Conversation

developit
Copy link
Member

@developit developit commented Dec 6, 2021

This changes the ordering of useEffect callbacks so that they fire roughlydeterministically in order of reverse tree depth (innermost to outermost). Callbacks will still fire in-order for multiple useEffect()'s within a single component, but the callbacks for a child component will always fire before those of a parent.

We already invoked useEffect() callbacks in reverse-depth-order for normal/full tree updates, since they were pushed into the queue from options.diffed() which fires inner-to-outer. With this PR, that ordering of useEffect callbacks is now also used when a parent subtree and child subtree are both enqueued for rendering, and the parent update does not descend to the child/grandchild (due to sCU/memo). In that case, we always render the parent component/tree before the child, but will now fire the child's useEffect callbacks before those of the parent.

h/t @wonderful-panda for uncovering this: https://twitter.com/wonderful_panda/status/1467015985672892416

We don't have specific timing that we state these calls will adhere to, so it seems like aligning with React makes the most sense here. React fires in-order within a component, but inner-to-outer between components (demo):

Screen Shot 2021-12-06 at 12 31 32 PM

This changes the ordering of useEffect callbacks so that they fire roughly in order of reverse component depth. Callbacks will still fire in-order for multiple useEffect()'s within a single component, but the callbacks for a child component will fire before those of a parent.
@github-actions
Copy link

github-actions bot commented Dec 6, 2021

Size Change: +70 B (0%)

Total Size: 42.2 kB

Filename Size Change
hooks/dist/hooks.js 1.17 kB +23 B (1%)
hooks/dist/hooks.module.js 1.18 kB +25 B (2%)
hooks/dist/hooks.umd.js 1.24 kB +22 B (1%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.46 kB 0 B
compat/dist/compat.module.js 3.44 kB 0 B
compat/dist/compat.umd.js 3.51 kB 0 B
debug/dist/debug.js 2.99 kB 0 B
debug/dist/debug.module.js 2.98 kB 0 B
debug/dist/debug.umd.js 3.07 kB 0 B
devtools/dist/devtools.js 232 B 0 B
devtools/dist/devtools.module.js 240 B 0 B
devtools/dist/devtools.umd.js 308 B 0 B
dist/preact.js 3.97 kB 0 B
dist/preact.min.js 4 kB 0 B
dist/preact.module.js 3.99 kB 0 B
dist/preact.umd.js 4.03 kB 0 B
jsx-runtime/dist/jsxRuntime.js 317 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 327 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 395 B 0 B
test-utils/dist/testUtils.js 437 B 0 B
test-utils/dist/testUtils.module.js 439 B 0 B
test-utils/dist/testUtils.umd.js 515 B 0 B

compressed-size-action

@JoviDeCroock
Copy link
Member

@developit I think we're already executing in the right order https://codesandbox.io/s/snowy-wave-jfz5s?file=/src/index.js

@JoviDeCroock JoviDeCroock self-requested a review December 7, 2021 19:29
@developit
Copy link
Member Author

@JoviDeCroock ah, I think I see why - we push into the afterPaintCallback queue from within options.diffed, which is called inner-to-outer. Good catch.

@developit developit closed this Dec 7, 2021
@JoviDeCroock JoviDeCroock deleted the useeffect-reverse-depth-ordering branch December 7, 2021 19:36
@developit
Copy link
Member Author

developit commented Dec 7, 2021

Ah - figured it out: this affects updates when components are memoized or bail out via sCU.
In this modified version of the demo, triggering an update of Inner + Outer in the same batch will fire the outer callback before the inner one:
https://codesandbox.io/s/wizardly-tdd-qwz16?file=/src/index.js

I believe this is why only two tests are red from the change - we were already calling inner-to-outer in the majority of cases, this only affects the case where (state-based) updates are scheduled for both a subtree and its (grand)parent.

@developit developit restored the useeffect-reverse-depth-ordering branch December 7, 2021 19:52
@developit developit reopened this Dec 7, 2021
@developit
Copy link
Member Author

#3356 (review)

@developit developit reopened this Dec 7, 2021
@developit developit marked this pull request as ready for review December 7, 2021 20:22
@coveralls
Copy link

coveralls commented Dec 7, 2021

Coverage Status

Coverage increased (+0.001%) to 99.627% when pulling a496224 on useeffect-reverse-depth-ordering into 70bc35a on master.

@JoviDeCroock JoviDeCroock merged commit 8ce72fc into master Dec 7, 2021
@JoviDeCroock JoviDeCroock deleted the useeffect-reverse-depth-ordering branch December 7, 2021 21:07
@andrewiggins andrewiggins mentioned this pull request Feb 1, 2022
Skaiir added a commit to bpmn-io/form-js that referenced this pull request Sep 29, 2024
Skaiir added a commit to bpmn-io/form-js that referenced this pull request Sep 29, 2024
Skaiir added a commit to bpmn-io/form-js that referenced this pull request Sep 29, 2024
Skaiir added a commit to bpmn-io/form-js that referenced this pull request Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants