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

Track appear effect handover per element due to Suspense #2810

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

kurtextrem
Copy link
Contributor

@kurtextrem kurtextrem commented Sep 23, 2024

Context

With nested Suspense boundaries, it might be that any children mount at a later timing than queueMicrotask guarantees. While we could switch to another timing-based method, it doesn't reliably ensure the call to completeHandoff happens after all children with optimized appear effects have mounted and run their mount useEffect. This means, if some child A mounted and has run an optimized appear effect, it would complete the handoff and make all further components (or children) falsely think they didn't have an optimized appear effect, which could lead to re-running of the appear effects on mount.

The fix

We now store the handover state per appear optimization element id, and only based on that state decide if we want to run further animations only from motion inside useEffect.

With nested Suspense boundaries, it might be that any children mount at a later timing than `queueMicrotask` guarantees. While we could switch to another timing-based method, it doesn't reliably ensure our call happens later than the mounting of the optimized appear effect. There seems to be no reliable way of telling when those nested boundaries have mounted (after completing) hydration, so removing the functionality seems to be the most sensible way. We remove optimized appear animations from the appearAnimationStore anyway after running, so this was just a minor optimization.
The trade-off might be, appear effects re-run when changing breakpoints, but to me that sounds ok, as barely any user ever changes the breakpoint.
@mattgperry
Copy link
Collaborator

This line checks if handoff is complete. If it is, we know that the animation is being interrupted by a subsequent animation, so we cancel the optimised animation and start the next animation from now. This is the source of the observed bug.

So if different components are handing off at different times, rather than keep the completion status of all components globally we would want to keep a boolean for each individual component.

@kurtextrem
Copy link
Contributor Author

Hmm, so the handoff completed check is more like a "optimized appear animation started or running" check?

@kurtextrem kurtextrem marked this pull request as draft September 24, 2024 11:26
@kurtextrem kurtextrem marked this pull request as ready for review September 24, 2024 12:02
@kurtextrem kurtextrem changed the title Remove scheduleHandoffComplete due to Suspense Track scheduleHandoffComplete per element due to Suspense Sep 24, 2024
@kurtextrem kurtextrem changed the title Track scheduleHandoffComplete per element due to Suspense Track appear effect handover per element due to Suspense Sep 24, 2024
@kurtextrem
Copy link
Contributor Author

Updated:

  • added a test (fails on master as expected, doesn't fail on this branch)
  • we now track the handover state per element (updated description), this is cheap as the only meaningful change here is Set -> Map

@kurtextrem kurtextrem force-pushed the remove-complete-handoff branch from 6d257bd to 6495100 Compare September 24, 2024 12:18
@kurtextrem kurtextrem force-pushed the remove-complete-handoff branch from 6495100 to 8d24787 Compare September 30, 2024 11:07
@mattgperry mattgperry merged commit 7eb6728 into main Oct 4, 2024
1 check passed
@mattgperry mattgperry deleted the remove-complete-handoff branch October 4, 2024 11:17
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.

2 participants