Always call LayoutGroup effect destroy functions.

[smoores-dev]

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!

[tilgovi]

Don't the deleted lines 57-60 destroy the layout effects?

[smoores-dev]

Yes, this now happens on line 43, instead (which is executed as a layout effect cleanup function in the child component)

[tilgovi]

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?

[smoores-dev]

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:

• I actually don't think it's surprising; the behavior is now that the create effect runs after the view create effect, and the destroy effect runs before it. This actually is what I had been assuming was happening before I looked into this, and I think it makes some intuitive sense
• React itself is inconsistent in exactly this way, so I think we get a pass! Haha

[smoores-dev]

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?

[smoores-dev]

Just added a second commit to demonstrate the alternative approach!

[tilgovi]

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?

I was just about to suggest something like this, but let me take a bit to digest.

[smoores-dev]

Done!

[smoores-dev]

Oh, wait, maybe I should have checked; were you saying we should go ahead with approach 1 (where we never queue) or 2 (where we always queue unless we're unmounting)?

OH. Drop the comment. Hm, yeah ok, I'm fine with that I suppose.

[smoores-dev]

Ok now done!

[tilgovi]

👏🏻