Fix async batching in React.startTransition #29226
Merged
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 4c2e457...12a387d 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
fix-async-batching-starttransition
branch
from
69d94fc
to
56f7017
Compare
Discovered a flaw in the current implementation of the isomorphic
startTransition implementation (React.startTransition), related to async
actions. It only creates an async scope if something calls setState
within the synchronous part of the action (i.e. before the first
`await`). I had thought this was fine because if there's no update
during this part, then there's nothing that needs to be entangled. I
didn't think this through, though — if there are multiple async updates
interleaved throughout the rest of the action, we need the async scope
to have already been created so that _those_ are batched together.
An even easier way to observe this is to schedule an optimistic update
after an `await` — the optimistic update should not be reverted until
the async action is complete.
To implement, during the reconciler's module initialization, we compose
its startTransition implementation with any previous reconciler's
startTransition that was already initialized. Then, the isomorphic
startTransition is the composition of every
reconciler's startTransition.
function startTransition() {
startTransitionDOM(() => {
startTransitionART(() => {
startTransitionThreeFiber(() => {
// and so on...
});
});
});
});
This is basically how flushSync is implemented, too.
acdlite
force-pushed
the
fix-async-batching-starttransition
branch
from
56f7017
to
12a387d
Compare
gnoff
approved these changes
May 23, 2024
Comment on lines
+76
to
+78
| if (prevOnStartTransitionFinish !== null) { | ||
| prevOnStartTransitionFinish(transition, returnValue); | ||
| } |
There was a problem hiding this comment.
For flushSync and friends we went with a noop default function so we don't have to do this type check on every invocation
There was a problem hiding this comment.
Is calling a noop function less work than a null check?
There was a problem hiding this comment.
@sebmarkbage had some concern over the runtime's ability to optimize it but I don't recall the specifics
Comment on lines
+46
to
+49
| const onStartTransitionFinish = ReactSharedInternals.S; | ||
| if (onStartTransitionFinish !== null) { | ||
| onStartTransitionFinish(transition, returnValue); | ||
| } |
There was a problem hiding this comment.
Do we need to account for cases like
require('react').startTransition(() => {
require('react-dom').createRoot(...).render(<App />)
})
I suppose the shared internals would be mutated by the time we reference the onStartTransitionFinish but maybe if you did the require after something async it would be too late?
There was a problem hiding this comment.
Unlike with flushSync I don't think there's a great way to account for the async case. The non-async case also wouldn't work if we were to move the isTransition logic off the shared internals and into the reconciler only, which is probably what we'll eventually do. I think it's fine to leave as a wont-fix.
github-actions bot
pushed a commit
that referenced
this pull request
May 23, 2024
Note: Despite the similar-sounding description, this fix is unrelated to
the issue where updates that occur after an `await` in an async action
must also be wrapped in their own `startTransition`, due to the absence
of an AsyncContext mechanism in browsers today.
---
Discovered a flaw in the current implementation of the isomorphic
startTransition implementation (React.startTransition), related to async
actions. It only creates an async scope if something calls setState
within the synchronous part of the action (i.e. before the first
`await`). I had thought this was fine because if there's no update
during this part, then there's nothing that needs to be entangled. I
didn't think this through, though — if there are multiple async updates
interleaved throughout the rest of the action, we need the async scope
to have already been created so that _those_ are batched together.
An even easier way to observe this is to schedule an optimistic update
after an `await` — the optimistic update should not be reverted until
the async action is complete.
To implement, during the reconciler's module initialization, we compose
its startTransition implementation with any previous reconciler's
startTransition that was already initialized. Then, the isomorphic
startTransition is the composition of every
reconciler's startTransition.
```js
function startTransition(fn) {
return startTransitionDOM(() => {
return startTransitionART(() => {
return startTransitionThreeFiber(() => {
// and so on...
return fn();
});
});
});
}
```
This is basically how flushSync is implemented, too.
DiffTrain build for [ee5c194](ee5c194)
github-actions bot
pushed a commit
that referenced
this pull request
May 23, 2024
Note: Despite the similar-sounding description, this fix is unrelated to
the issue where updates that occur after an `await` in an async action
must also be wrapped in their own `startTransition`, due to the absence
of an AsyncContext mechanism in browsers today.
---
Discovered a flaw in the current implementation of the isomorphic
startTransition implementation (React.startTransition), related to async
actions. It only creates an async scope if something calls setState
within the synchronous part of the action (i.e. before the first
`await`). I had thought this was fine because if there's no update
during this part, then there's nothing that needs to be entangled. I
didn't think this through, though — if there are multiple async updates
interleaved throughout the rest of the action, we need the async scope
to have already been created so that _those_ are batched together.
An even easier way to observe this is to schedule an optimistic update
after an `await` — the optimistic update should not be reverted until
the async action is complete.
To implement, during the reconciler's module initialization, we compose
its startTransition implementation with any previous reconciler's
startTransition that was already initialized. Then, the isomorphic
startTransition is the composition of every
reconciler's startTransition.
```js
function startTransition(fn) {
return startTransitionDOM(() => {
return startTransitionART(() => {
return startTransitionThreeFiber(() => {
// and so on...
return fn();
});
});
});
}
```
This is basically how flushSync is implemented, too.
DiffTrain build for commit ee5c194.
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.
Note: Despite the similar-sounding description, this fix is unrelated to the issue where updates that occur after an
awaitin an async action must also be wrapped in their ownstartTransition, due to the absence of an AsyncContext mechanism in browsers today.Discovered a flaw in the current implementation of the isomorphic startTransition implementation (React.startTransition), related to async actions. It only creates an async scope if something calls setState within the synchronous part of the action (i.e. before the first
await). I had thought this was fine because if there's no update during this part, then there's nothing that needs to be entangled. I didn't think this through, though — if there are multiple async updates interleaved throughout the rest of the action, we need the async scope to have already been created so that those are batched together.An even easier way to observe this is to schedule an optimistic update after an
await— the optimistic update should not be reverted until the async action is complete.To implement, during the reconciler's module initialization, we compose its startTransition implementation with any previous reconciler's startTransition that was already initialized. Then, the isomorphic startTransition is the composition of every
reconciler's startTransition.
This is basically how flushSync is implemented, too.