Call cleanup of insertion effects when hidden#30954
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Comparing: 94e4acaa1477e65cac02ba86058cde0afe4c8f1f...a99a92c7387bf6151666d678dd4491a7bc3e7eac Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
4b19a8e to
d2c38f4
Compare
Insertion effects do not unmount when a subtree goes offscreen, this means we still have to traverse the subtree in that case. Likely also fixes react#26670
|
This is intentional. We should not cleanup insertion effects when hidden. Additionally they should actually run early inside a hidden tree if it commits in the hidden state. That's because otherwise we cannot perform layout calculation warm ups of the hidden tree. (Insertion effects is really just code for "insert css-in-js style tags". Really it should be deleted eventually once all those libraries have moved to Float.) |
|
Another example is portals. useInsertionEffect can be used to insert a hidden portal early and have it perform its layout calculations and possibly even prerendering. Then useLayoutEffect should reveal it. So for a typical mount it gets inserted as hidden then immediately revealed. Then the useLayoutEffect unmounts it can be hidden but still in the DOM. Allowing the layout state and DOM state to be preserved just like a non-portal would. |
|
@sebmarkbage Ah I think there's a misunderstanding (@kassens it might be worth rephrasing the description a bit). What we've observed is:
As you noted it's expected that it shouldn't unmount the effect in (2). But we would expect it to unmount in (3). |
|
(sorry for the imprecise description, I'll update it) This does not change when they're mounted, just ensures they're unmounted if a hidden subtree is unmounted. Current behavior for an insertion effect is if the component goes
This change is supposed to make it consistently call insertion effect cleanup upon removing the component (not when just offscreen), even if going through hidden first. The other option for consistency would be to say insertion effects never clean up, but that's a bigger behavior change. |
|
Oh I see |
|
@sebmarkbage I added more tests so it's more clear which unmounts are actually added, and adds more coverage for the existing behavior. Does this make sense? It should only unmount the insertion effects when the tree is deleted, but still not when it it's only hidden. @kassens I also added the error if you call setState in useInsertionEffect unmount for this case. |
josephsavona
left a comment
There was a problem hiding this comment.
Looks good, just one nit
Insertion effects do not unmount when a subtree is removed while offscreen. Current behavior for an insertion effect is if the component goes - *visible -> removed:* calls insertion effect cleanup - *visible -> offscreen -> removed:* insertion effect cleanup is never called This makes it so we always call insertion effect cleanup when removing the component. Likely also fixes #26670 --------- Co-authored-by: Rick Hanlon <rickhanlonii@fb.com> DiffTrain build for commit d3d4d3a.
Insertion effects do not unmount when a subtree is removed while offscreen. Current behavior for an insertion effect is if the component goes - *visible -> removed:* calls insertion effect cleanup - *visible -> offscreen -> removed:* insertion effect cleanup is never called This makes it so we always call insertion effect cleanup when removing the component. Likely also fixes #26670 --------- Co-authored-by: Rick Hanlon <rickhanlonii@fb.com> DiffTrain build for [d3d4d3a](d3d4d3a)
**breaking change for canary users: Bumps peer dependency of React from `19.0.0-rc-206df66e-20240912` to `19.0.0-rc-a99d8e8d-20240916`** [diff react/react@206df66e...a99d8e8d](react/react@206df66...a99d8e8) <details> <summary>React upstream changes</summary> - react/react#30977 - react/react#30971 - react/react#30922 - react/react#30917 - react/react#30902 - react/react#30912 - react/react#30970 - react/react#30969 - react/react#30967 - react/react#30966 - react/react#30960 - react/react#30968 - react/react#30961 - react/react#28255 - react/react#30957 - react/react#30958 - react/react#30959 - react/react#30951 - react/react#30954 - react/react#30920 - react/react#30942 </details>
Insertion effects do not unmount when a subtree is removed while offscreen.
Current behavior for an insertion effect is if the component goes
This makes it so we always call insertion effect cleanup when removing the component.
Likely also fixes #26670