Solidify addTransitionType Semantics #32797
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
React Core Team
|
Comparing: deca965...25af4b8 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
|
sebmarkbage
force-pushed
the
addtransitionrefactor
branch
from
a028dc0 to
403a153
Compare
rickhanlonii
approved these changes
Apr 1, 2025
There was a problem hiding this comment.
At some point (not now) it would be nice to have tests to reference as specs for the behavior. For this one I think we could add Scheduler.log in the ViewTransition event to assert on the types if we shimmed startViewTransition?
Don't want to slow this down yet though, this is a good example of a change harder to make if you have to update a bunch of tests too.
Reviewed: sebmarkbage/react@f57175d...3399b16
sebmarkbage
force-pushed
the
addtransitionrefactor
branch
from
403a153 to
21cd559
Compare
Conceptually they belong to each Transition object and should only be applied where updates to that batch are applied. However, since we batch regular Transitions together they go into the same set.
startTransition(() => {
startTransition(() => {
addTransitionType(...)
});
setState(...);
});
When the inner one finishes you could drop its transition types because it
has no work scheduled on it.
We store them on all roots that have been scheduled as part of the startTransition call. We shouldn't just let the first root that commits steal them all. Each root gets its own set.
…pes to every root that gets scheduled
sebmarkbage
force-pushed
the
addtransitionrefactor
branch
from
21cd559 to
25af4b8
Compare
sebmarkbage
added a commit
that referenced
this pull request
Apr 1, 2025
…Map (#32800) We have a high level concept for this used elsewhere. We should use this for `transitionTypes` too: https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactInternalTypes.js#L285 As mentioned in #32797 we could also just use the `transitionLanes` since the `types` are also on the `Transition` objects. If we always stored this set.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 1, 2025
…Map (#32800) We have a high level concept for this used elsewhere. We should use this for `transitionTypes` too: https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactInternalTypes.js#L285 As mentioned in #32797 we could also just use the `transitionLanes` since the `types` are also on the `Transition` objects. If we always stored this set. DiffTrain build for [450f8df](450f8df)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stacked on #32793.
This is meant to model the intended semantics of
addTransitionTypebetter. The previous hack just consumed all transition types when any root committed so it could steal them from other roots. Really each root should get its own set. Really each transition lane should get its own set.We can't implement the full ideal semantics yet because 1) we currently entangle transition lanes 2) we lack
AsyncContexton the client so for async actions we can't associate aaddTransitionTypecall to a specificstartTransition.This starts by modeling Transition Types to be stored on the Transition instance. Conceptually they belong to the Transition instance of that
startTransitionthey belong to. That instance is otherwise mostly just used for Transition Tracing but it makes sense that those would be able to be passed the Transition Types for that specific instance.Nested
startTransitionneed to get entangled. So that thisaddTransitionTypecan be associated with thesetState:Ideally we'd probably just use the same Transition instance itself since these are conceptually all part of one entangled one. But transition tracing uses multiple names and start times. Unclear what we want to do with that. So I kept separate instances but shared
typesset.Next I collect the types added during a
startTransitionto any root scheduled with a Transition. This should really be collected one set per Transition lane in aLaneMap. In fact, the information would already be there if Transition Tracing was always enabled because it tracks all Transition instances per lane. For now I just keep track of one set for all Transition lanes. Maybe we should only add it if asetStatewas done on this root in this particularstartTransitioncall rather having already scheduled any Transition earlier.While async transitions are entangled, we don't know if there will be a startTransition+setState on a new root in the future. Therefore, we collect all transition types while this is happening and if a new root gets startTransition+setState they get added to that root.