-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Noop unstable_batchedUpdates #28120
Noop unstable_batchedUpdates #28120
Conversation
c6a06f7
to
4c4b9ae
Compare
Comparing: 9f33f69...9d279d2 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
|
34d6e25
to
f792e8f
Compare
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.
This makes the public API a no-op but React DOM's internals also call the batchedUpdates
function exposed by the reconciler.
batchedUpdates, |
So for maximum safety I think we should fork the implementation here. That way we don't accidentally rely on the executionContext
being set:
react/packages/react-reconciler/src/ReactFiberWorkLoop.js
Lines 1436 to 1455 in 2efa383
export function batchedUpdates<A, R>(fn: A => R, a: A): R { | |
const prevExecutionContext = executionContext; | |
executionContext |= BatchedContext; | |
try { | |
return fn(a); | |
} finally { | |
executionContext = prevExecutionContext; | |
// If there were legacy sync updates, flush them at the end of the outer | |
// most batchedUpdates-like method. | |
if ( | |
executionContext === NoContext && | |
// Treat `act` as if it's inside `batchedUpdates`, even in legacy mode. | |
!(__DEV__ && ReactCurrentActQueue.isBatchingLegacy) | |
) { | |
resetRenderTimer(); | |
flushSyncWorkOnLegacyRootsOnly(); | |
} | |
} | |
} | |
I was thinking we would add a Fiber config like supportsLegacyMode
and check that inside batchedUpdates
.
We could also add a check in React DOM to remove the extra stack frame, too, but I would still fork the lower-level function just in case. Makes it easier to delete that code later when we delete the rest of legacy mode, too. |
f792e8f
to
e06ca02
Compare
Oh, thanks @acdlite I didn't realize we also wanted to noop the internal batched updates. I updated to use a feature flag. This means www will still use the real batchedUpdates even in createRoot, but that's how it works today and I can't figure out the right layering to prevent it. We could merge and follow up if it's a concern? |
There is a related task to move all the client apis into I could implement it using a dispatcher of sorts but I'd like to avoid it if possible however I'll need the dispatcher for flushSync so it's not like I get to completely skip that whole concept even if we do go with this strategy |
6de030c
to
b383fae
Compare
b383fae
to
61c03f5
Compare
61c03f5
to
9d279d2
Compare
@@ -248,7 +248,7 @@ describe('ReactMount', () => { | |||
expect(calls).toBe(5); | |||
}); | |||
|
|||
// @gate !disableLegacyMode | |||
// @gate !disableLegacyMode && classic |
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.
Since this no-ops in every entry point except www-classic, these tests need the extra gate.
## Overview `unstable_batchedUpdates` is effectively a no-op outside of legacy mode, this PR makes it an actual no-op outside legacy mode.
## Overview `unstable_batchedUpdates` is effectively a no-op outside of legacy mode, this PR makes it an actual no-op outside legacy mode. DiffTrain build for commit 63651c4.
Overview
unstable_batchedUpdates
is effectively a no-op outside of legacy mode, this PR makes it an actual no-op outside legacy mode.