Always call LayoutGroup effect destroy functions. #103
Conversation
|
Don't the deleted lines 57-60 destroy the layout effects? |
|
Yes, this now happens on line 43, instead (which is executed as a layout effect cleanup function in the child component) |
|
I don't have a use case in mind where grouping destroys is actually important, but the asymmetry makes me uncomfortable. It feels more surprising somehow. I think what's maybe important is that if a descendant of the group unmounts, it should trigger a flush of the group, because the other layout effects of descendants that continue to exist might need to update. Consider a scenario where some other component in the tree has a layout group effect with no dependencies. We want to force the whole group to rerender so that re-runs after the unmount of another component, no? |
|
If I'm understanding correctly, we were already trying to do what you're describing with line 46. It's not sufficient, because the parent has already unmounted by the time the trigger to flush runs, and so it does not re-run its cleanup effect. I agree that this is inconsistent, but:
|
|
There is another approach where we keep the destroyQueue as is, and special-case the scenario where the LayoutGroup is unmounted. This would allow us to actually be more consistent than React is, and always run child effect destroy functions after the LayoutGroup's. Does that seem better? |
|
Just added a second commit to demonstrate the alternative approach! |
I was just about to suggest something like this, but let me take a bit to digest. |
src/components/LayoutGroup.tsx
Outdated
| @@ -18,6 +18,7 @@ export interface LayoutGroupProps { | |||
| export function LayoutGroup({ children }: LayoutGroupProps) { | |||
| const createQueue = useRef(new Set<() => void>()).current; | |||
| const destroyQueue = useRef(new Set<() => void>()).current; | |||
| const isMounted = useRef(true); | |||
There was a problem hiding this comment.
Is this actually true, initially? 😆
There was a problem hiding this comment.
I meeeaaannnn... I guess it depends on your definition of mounted haha. Given the way that this is being used, I suppose it's safe to set this to false initially and set it true in a useLayoutEffect creator.
There was a problem hiding this comment.
Yeah. If somehow a child were to mount and unmount, due to synchronous updates forced by state changes done in layout effects further down the tree, we might want that child to destroy its effect? But I guess it wouldn't actually have created its effect yet. Huh. I dunno.
This is so unfortunately complicated to reason about!
It might be that I just got this wrong, initially. I maybe assumed that cleanup was child->parent, in the same way that effect creation is. But it might be parent->child, even during regular updates?
There was a problem hiding this comment.
If so, I'm tempted by the idea of not queueing child destroys, but providing a simpler explanation for why.
There was a problem hiding this comment.
It's not, I've checked. It is child->parent in regular updates, and actually behaves differently and runs parent->child in during unmounts!
There was a problem hiding this comment.
That's true even if both the parent and the child have effects with dependencies that changed and the render was caused by a state update at the parent or above?
There was a problem hiding this comment.
I was testing with effects with no dependencies because it seemed easier to coordinate. But yes, if you use effects with no dependencies, and update the state at the parent, effect cleanups are run from child -> parent. If you unmount the parent, they are run from parent -> child
There was a problem hiding this comment.
For anyone that wants to follow along, here's a minimal reproduction on CodeSandbox.
If you open the console, you'll see that the order of the print statements changes depending on whether you click Update or Toggle mount!
smoores-dev
force-pushed
the
fix-layout-group-destroy
branch
from
c5a3f10
to
f860a7f
Compare
There was a problem hiding this comment.
Let's do this!
- Split this into two PRs, one for the regression and one for the fix to layout group effects.
- Only set
isMountedonce we're actually mounted. - Maybe you can drop the comment around why we destroy without a queue. That'd cause me to scratch my head when simply seeing that we're not mounted is enough for me to know why can't just flush again.
smoores-dev
force-pushed
the
fix-layout-group-destroy
branch
from
f860a7f
to
4d204fa
Compare
|
Done! |
smoores-dev
force-pushed
the
fix-layout-group-destroy
branch
from
4d204fa
to
73792ad
Compare
|
OH. Drop the comment. Hm, yeah ok, I'm fine with that I suppose. |
This resolves an issue where LayoutGroup effects were not cleaned up when the LayoutGroup itself was unmounted. React executes layout effect destroy functions from parent to child on unmount, which is opposite how it runs during normal render. The result was that the destroyQueue was never processed on unmount. Now, if the LayoutGroup is being unmounted, we call destroy functions immediately, without queuing.
smoores-dev
force-pushed
the
fix-layout-group-destroy
branch
from
73792ad
to
6f7bb51
Compare
|
Ok now done! |
|
👏🏻 |
This resolves an issue where LayoutGroup effects were not cleaned up when the LayoutGroup itself was unmounted. React executes layout effect destroy functions from parent to child on unmount, which is opposite how it runs during normal render. The result was that the destroyQueue was never processed on unmount.
Now, if the LayoutGroup is being unmounted, we call destroy functions immediately, without queuing.
Note: This also fixes the Prettier config for VS Code, which got messed up when we upgraded to v3!