-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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
Support nesting of React.startTransition and ReactDOM.flushSync #21136
Closed
acdlite
wants to merge
1
commit into
facebook:master
from
acdlite:support-nesting-starttransition-flushsync
Closed
Support nesting of React.startTransition and ReactDOM.flushSync #21136
acdlite
wants to merge
1
commit into
facebook:master
from
acdlite:support-nesting-starttransition-flushsync
Conversation
This file contains 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
Currently we track event priorities in two different places: - For transitions, we use the ReactCurrentBatchConfig object, which lives in the secret internals object of the isomorphic package. This is how the hook-less `startTransition` works. - For other types of events, we use a module-level variable in ReactEventPriorities, which is owned by the renderer. The problem with this approach is if both priorities are set, we don't know which one takes precedence. For example, what happens if wrap an update in both `ReactDOM.flushSync` and `React.startTransition`? The desired behavior is that we use the priority of whichever wrapper is closer on the stack. So if `flushSync` is called inside of `startTransition`, then we should use sync priority; if it's the other way around, we should use transition priority. To implement this, I updated the ReactEventPriorities module to track priority on the ReactCurrentBatchConfig object instead of in a module- level variable. Now when multiple event wrappers are nested, the inner- most wrapper wins.
facebook-github-bot
added
CLA Signed
React Core Team
Opened by a member of the React Core Team
labels
Mar 30, 2021
Comparing: 0853aab...16a8eca 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
|
acdlite
force-pushed
the
support-nesting-starttransition-flushsync
branch
2 times, most recently
from
March 30, 2021 03:57
6d7dc04
to
f2a4e8c
Compare
acdlite
commented
Mar 30, 2021
acdlite
force-pushed
the
support-nesting-starttransition-flushsync
branch
3 times, most recently
from
March 31, 2021 04:02
9d2d689
to
16a8eca
Compare
acdlite
changed the title
RFC: Support nesting of React.startTransition and ReactDOM.flushSync
Support nesting of React.startTransition and ReactDOM.flushSync
Mar 31, 2021
rickhanlonii
approved these changes
Mar 31, 2021
// boundary: importing from a shared module would give a false sense of | ||
// DRYness, because it's theoretically possible for for the renderer and | ||
// the isomorphic package to be out of sync. We don't fully support that, but we | ||
// should try (within reason) to be resilient. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, copying over gives a false sense of safety since if the renderer and isomorphic are out of sync, this could also be wrong.
Closed in favor of #21149 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently we track event priorities in two different places:
startTransition
works.The problem with this approach is if both priorities are set, we don't know which one takes precedence. For example, what happens if wrap an update in both
ReactDOM.flushSync
andReact.startTransition
?The desired behavior is that we use the priority of whichever wrapper is closer on the stack. So if
flushSync
is called inside ofstartTransition
, then we should use sync priority; if it's the other way around, we should use transition priority.To implement this, I updated the ReactEventPriorities module to track priority on the ReactCurrentBatchConfig object instead of in a module- level variable. Now when multiple event wrappers are nested, the innermost wrapper wins.