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

Noop unstable_batchedUpdates #28120

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Conversation

rickhanlonii
Copy link
Member

@rickhanlonii rickhanlonii commented Jan 27, 2024

Overview

unstable_batchedUpdates is effectively a no-op outside of legacy mode, this PR makes it an actual no-op outside legacy mode.

@react-sizebot
Copy link

react-sizebot commented Jan 27, 2024

Comparing: 9f33f69...9d279d2

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.01% 176.93 kB 176.95 kB +0.02% 54.99 kB 55.01 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 173.11 kB 173.05 kB = 53.95 kB 53.95 kB
facebook-www/ReactDOM-prod.classic.js = 592.73 kB 592.75 kB +0.01% 103.97 kB 103.98 kB
facebook-www/ReactDOM-prod.modern.js +0.02% 574.31 kB 574.44 kB +0.02% 100.99 kB 101.00 kB
test_utils/ReactAllWarnings.js Deleted 65.19 kB 0.00 kB Deleted 16.30 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 65.19 kB 0.00 kB Deleted 16.30 kB 0.00 kB

Generated by 🚫 dangerJS against 9d279d2

@rickhanlonii rickhanlonii force-pushed the rh/batched-noop branch 2 times, most recently from 34d6e25 to f792e8f Compare January 29, 2024 22:47
Copy link
Collaborator

@acdlite acdlite left a 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.

So for maximum safety I think we should fork the implementation here. That way we don't accidentally rely on the executionContext being set:

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.

packages/react-dom/src/client/ReactDOM.js Show resolved Hide resolved
@acdlite
Copy link
Collaborator

acdlite commented Feb 1, 2024

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.

@rickhanlonii
Copy link
Member Author

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?

@gnoff
Copy link
Collaborator

gnoff commented Feb 7, 2024

There is a related task to move all the client apis into react-dom/client and make react-dom not import any of the client reconciler. I was hoping I could just make batched updates a noop there and not even import from react-reconciler however this would mean the implementation used internally would differ from the public API for legacy mode. I think we could solve this by exposing the reconciler version of batched updates on the meta entrypoints and using the basic noop implementation on OOS entrypoints since the flag should align behavior in this case.

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

@rickhanlonii rickhanlonii marked this pull request as draft March 26, 2024 17:06
@rickhanlonii rickhanlonii force-pushed the rh/batched-noop branch 3 times, most recently from 6de030c to b383fae Compare March 27, 2024 21:27
@rickhanlonii rickhanlonii marked this pull request as ready for review March 27, 2024 21:35
@@ -248,7 +248,7 @@ describe('ReactMount', () => {
expect(calls).toBe(5);
});

// @gate !disableLegacyMode
// @gate !disableLegacyMode && classic
Copy link
Member Author

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.

@rickhanlonii rickhanlonii merged commit 63651c4 into facebook:main Mar 28, 2024
38 checks passed
@rickhanlonii rickhanlonii deleted the rh/batched-noop branch March 28, 2024 18:15
github-actions bot pushed a commit that referenced this pull request Mar 28, 2024
## 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 [63651c4](63651c4)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
## Overview

`unstable_batchedUpdates` is effectively a no-op outside of legacy mode,
this PR makes it an actual no-op outside legacy mode.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
## 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants