Destroy insertion effects when deleting previously hidden subtrees #26843
Conversation
|
Comparing: 4b877b6...905106c Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| destroy, | ||
| ); | ||
| } else if ( | ||
| !offscreenSubtreeWasHidden && |
There was a problem hiding this comment.
Effectively I just moved the !offscreenSubtreeWasHidden check here which caused the whole block to be dedented.
- this code is responsible for destroying insertion and layout effects (passive effects are destroyed in
commitPassiveUnmountOnFiber) - layout effects are guaranteed to be already destroyed when
offscreenSubtreeWasHiddenbecause they are destroyed bydisappearLayoutEffectsimmediately~ when the tree gets hidden. We can't destroy them here unconditionally because that would sometimes destroy them twice - and yet we need to destroy insertion effects here since they are meant to stay "connected" when the tree gets hidden (that's what @gaearon suggested to me as the fix for this issue). That's why this loop through
updateQueueis not completely avoidable here right now.
| 'Text:Function destroy layout', | ||
| 'ClassText:Class componentWillUnmount', | ||
| 'Text:Function destroy passive', | ||
| ]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('insertion effects within a tree that re-suspends in an update', () => { | ||
| // @gate enableLegacyCache | ||
| it('should be destroyed in the deleted tree', async () => { |
There was a problem hiding this comment.
this is the only new test here in this PR
| @@ -148,6 +148,12 @@ describe('ReactSuspenseEffectsSemantics', () => { | |||
|
|
|||
| function Text({children = null, text}) { | |||
| Scheduler.log(`Text:${text} render`); | |||
| React.useInsertionEffect(() => { | |||
There was a problem hiding this comment.
It made sense to me to add useInsertionEffect to all of those helper components that are used throughout this test suite.
It increases the coverage and might surface some subtle bugs in the future. I think that this matches the spirit of this file because tests that are explicitly concerned with layout effects behavior are also tracking passive effects (since they rely on those helper components). So I didn't see a reason why insertion effects should be skipped from here.
If you don't like the change - let me know and I can easily remove that and only leave the targeted test case for the fixed issue.
| 'AsyncText:Two render', | ||
| 'Text:Fallback destroy insertion', | ||
| 'Text:Fallback destroy layout', | ||
| 'Text:One destroy insertion', |
There was a problem hiding this comment.
This one position is what is missing without the fix.
I'm not completely sure how I feel about insertion effects staying connected in hidden subtrees. I think that goes against my intuition as I always thought about them as a layout effect variant. It made sense to me because their introduction is directly related to the needs of CSS-in-JS libraries.
We have used useInsertionEffect in Emotion's <Global/> (a component responsible for injecting global styles) and it seems that if those are the desired semantics for useInsertionEffect then we should reconsider that. We can't fully rely on the destroy callback being called at the right time here:
https://github.com/emotion-js/emotion/blob/1135f8e9d97ea711eb483368313afdfe7b176845/packages/react/src/global.js#L112-L114
I prepared a demo to showcase the problem with this patch applied to it (couldn't use codesandbox's per-commit builds since CSB is 431-ing me when I try to save a sandbox 😢):
https://github.com/Andarist/use-insertion-effect-stays-connected-when-hidden-demo/tree/e11c278fd69c947764d4d5c14399326bff6cdca2
What happens here:
- Page1 is being loaded (white background since we render fallback)
- Page1 loads and the background becomes red through the usage of Emotion's
<Global/> - we click the link and navigate to Page2, since it's a lazy component we display the fallback but the background stays red while the fallback is being rendered
- background finally switches to white when Page2 loads
I understand that this might not be how you have wanted us to use this but I'm not sure if this effect here is desirable for us and our users. If insertion effects are meant to be connected in hidden trees then I'll likely have to consider refactoring the <Global/> component.
Andarist
force-pushed
the
fix/insertion-effects-unmount
branch
from
6f57d62
to
905106c
Compare
|
@sammy-SC @acdlite @sebmarkbage @rickhanlonii friendly 🏓 |
|
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. |
github-actions
bot
added
the
Resolution: Stale
|
bump |
github-actions
bot
removed
the
Resolution: Stale
|
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. |
github-actions
bot
added
the
Resolution: Stale
|
bump |
github-actions
bot
removed
the
Resolution: Stale
|
From what I see almost the same core change landed as part of #30954 so I'm closing this one. |
|
Hey @Andarist, our apologies for missing this PR. Great find and thank you for submitting a fix! |
Summary
This PR fixes the lack of symmetry between creating and destroying insertion effects under certain circumstances. It could happen that the insertion effect was not destroyed if its owner component got hidden first as a result of re-suspending and if another subtree ended up being rendered.
How did you test this change?
fixes #26670
cc @gaearon