From 2b25548d99aba4d6c28c765422d53efe6d29758c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 30 Jun 2021 18:25:15 -0400 Subject: [PATCH] Replace unbatchedUpdates with flushSync There's a weird quirk leftover from the old Stack (pre-Fiber) implementation where the initial mount of a leagcy (ReactDOM.render) root is flushed synchronously even inside `batchedUpdates`. The original workaround for this was an internal method called `unbatchedUpdates`. We've since added another API that works almost the same way, `flushSync`. The only difference is that `unbatchedUpdates` would not cause other pending updates to flush too, only the newly mounted root. `flushSync` flushes all pending sync work across all roots. This was to preserve the exact behavior of the Stack implementation. But since it's close enough, let's just use `flushSync`. It's unlikely anyone's app accidentally relies on this subtle difference, and the legacy API is deprecated in 18, anyway. --- .../__snapshots__/profilingCache-test.js.snap | 28 +++--- .../src/__tests__/ReactMount-test.js | 12 +-- .../react-dom/src/client/ReactDOMLegacy.js | 6 +- packages/react-noop-renderer/src/ReactNoop.js | 1 - .../src/ReactNoopPersistent.js | 1 - .../src/createReactNoop.js | 2 - .../src/ReactFiberReconciler.js | 5 -- .../src/ReactFiberReconciler.new.js | 2 - .../src/ReactFiberReconciler.old.js | 2 - .../src/ReactFiberWorkLoop.new.js | 90 ++++--------------- .../src/ReactFiberWorkLoop.old.js | 90 ++++--------------- .../ReactIncrementalScheduling-test.js | 59 ------------ 12 files changed, 59 insertions(+), 239 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap index b27231efdffc7..2ab8751e3cbcb 100644 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap @@ -40,7 +40,7 @@ Object { 6 => 1, }, "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 16, "updaters": Array [ Object { @@ -87,7 +87,7 @@ Object { 4 => 2, }, "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 15, "updaters": Array [ Object { @@ -186,7 +186,7 @@ Object { 6 => 1, }, "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 12, "updaters": Array [ Object { @@ -445,7 +445,7 @@ Object { ], ], "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 12, "updaters": Array [ Object { @@ -938,7 +938,7 @@ Object { ], ], "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 11, "updaters": Array [ Object { @@ -1597,7 +1597,7 @@ Object { 17 => 1, }, "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 24, "updaters": Array [ Object { @@ -1687,7 +1687,7 @@ Object { "fiberActualDurations": Map {}, "fiberSelfDurations": Map {}, "passiveEffectDuration": 0, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 34, "updaters": Array [ Object { @@ -2223,7 +2223,7 @@ Object { ], ], "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 24, "updaters": Array [ Object { @@ -2310,7 +2310,7 @@ Object { "fiberActualDurations": Array [], "fiberSelfDurations": Array [], "passiveEffectDuration": 0, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 34, "updaters": Array [ Object { @@ -2431,7 +2431,7 @@ Object { 2 => 0, }, "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 0, "updaters": Array [ Object { @@ -2506,7 +2506,7 @@ Object { 3 => 0, }, "passiveEffectDuration": 0, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 0, "updaters": Array [ Object { @@ -2715,7 +2715,7 @@ Object { ], ], "passiveEffectDuration": 0, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 0, "updaters": Array [ Object { @@ -3071,7 +3071,7 @@ Object { 7 => 0, }, "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 0, "updaters": Array [ Object { @@ -3515,7 +3515,7 @@ Object { ], ], "passiveEffectDuration": null, - "priorityLevel": "Normal", + "priorityLevel": "Immediate", "timestamp": 0, "updaters": Array [ Object { diff --git a/packages/react-dom/src/__tests__/ReactMount-test.js b/packages/react-dom/src/__tests__/ReactMount-test.js index a214c32f58d06..9571905edaf52 100644 --- a/packages/react-dom/src/__tests__/ReactMount-test.js +++ b/packages/react-dom/src/__tests__/ReactMount-test.js @@ -277,7 +277,7 @@ describe('ReactMount', () => { expect(calls).toBe(5); }); - it('initial mount is sync inside batchedUpdates, but task work is deferred until the end of the batch', () => { + it('initial mount of legacy root is sync inside batchedUpdates, as if it were wrapped in flushSync', () => { const container1 = document.createElement('div'); const container2 = document.createElement('div'); @@ -302,12 +302,12 @@ describe('ReactMount', () => { // Initial mount on another root. Should flush immediately. ReactDOM.render(a, container2); - // The update did not flush yet. - expect(container1.textContent).toEqual('1'); - // The initial mount flushed, but not the update scheduled in cDM. - expect(container2.textContent).toEqual('a'); + // The earlier update also flushed, since flushSync flushes all pending + // sync work across all roots. + expect(container1.textContent).toEqual('2'); + // Layout updates are also flushed synchronously + expect(container2.textContent).toEqual('a!'); }); - // All updates have flushed. expect(container1.textContent).toEqual('2'); expect(container2.textContent).toEqual('a!'); }); diff --git a/packages/react-dom/src/client/ReactDOMLegacy.js b/packages/react-dom/src/client/ReactDOMLegacy.js index a62a7fb444c5b..f03350cb29240 100644 --- a/packages/react-dom/src/client/ReactDOMLegacy.js +++ b/packages/react-dom/src/client/ReactDOMLegacy.js @@ -29,7 +29,7 @@ import { createContainer, findHostInstanceWithNoPortals, updateContainer, - unbatchedUpdates, + flushSyncWithoutWarningIfAlreadyRendering, getPublicRootInstance, findHostInstance, findHostInstanceWithWarning, @@ -174,7 +174,7 @@ function legacyRenderSubtreeIntoContainer( }; } // Initial mount should not be batched. - unbatchedUpdates(() => { + flushSyncWithoutWarningIfAlreadyRendering(() => { updateContainer(children, fiberRoot, parentComponent, callback); }); } else { @@ -357,7 +357,7 @@ export function unmountComponentAtNode(container: Container) { } // Unmount should not be batched. - unbatchedUpdates(() => { + flushSyncWithoutWarningIfAlreadyRendering(() => { legacyRenderSubtreeIntoContainer(null, null, container, false, () => { // $FlowFixMe This should probably use `delete container._reactRootContainer` container._reactRootContainer = null; diff --git a/packages/react-noop-renderer/src/ReactNoop.js b/packages/react-noop-renderer/src/ReactNoop.js index 5e5567f9e0947..a225deaf961f1 100644 --- a/packages/react-noop-renderer/src/ReactNoop.js +++ b/packages/react-noop-renderer/src/ReactNoop.js @@ -38,7 +38,6 @@ export const { flushExpired, batchedUpdates, deferredUpdates, - unbatchedUpdates, discreteUpdates, idleUpdates, flushSync, diff --git a/packages/react-noop-renderer/src/ReactNoopPersistent.js b/packages/react-noop-renderer/src/ReactNoopPersistent.js index 97876990a9b57..86bb87c065e48 100644 --- a/packages/react-noop-renderer/src/ReactNoopPersistent.js +++ b/packages/react-noop-renderer/src/ReactNoopPersistent.js @@ -38,7 +38,6 @@ export const { flushExpired, batchedUpdates, deferredUpdates, - unbatchedUpdates, discreteUpdates, idleUpdates, flushDiscreteUpdates, diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index eb7e74193341c..5ea510d73d6ea 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -901,8 +901,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { deferredUpdates: NoopRenderer.deferredUpdates, - unbatchedUpdates: NoopRenderer.unbatchedUpdates, - discreteUpdates: NoopRenderer.discreteUpdates, idleUpdates(fn: () => T): T { diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index bf78e9e41c390..d25783164e984 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -18,7 +18,6 @@ import { createContainer as createContainer_old, updateContainer as updateContainer_old, batchedUpdates as batchedUpdates_old, - unbatchedUpdates as unbatchedUpdates_old, deferredUpdates as deferredUpdates_old, discreteUpdates as discreteUpdates_old, flushControlled as flushControlled_old, @@ -56,7 +55,6 @@ import { createContainer as createContainer_new, updateContainer as updateContainer_new, batchedUpdates as batchedUpdates_new, - unbatchedUpdates as unbatchedUpdates_new, deferredUpdates as deferredUpdates_new, discreteUpdates as discreteUpdates_new, flushControlled as flushControlled_new, @@ -99,9 +97,6 @@ export const updateContainer = enableNewReconciler export const batchedUpdates = enableNewReconciler ? batchedUpdates_new : batchedUpdates_old; -export const unbatchedUpdates = enableNewReconciler - ? unbatchedUpdates_new - : unbatchedUpdates_old; export const deferredUpdates = enableNewReconciler ? deferredUpdates_new : deferredUpdates_old; diff --git a/packages/react-reconciler/src/ReactFiberReconciler.new.js b/packages/react-reconciler/src/ReactFiberReconciler.new.js index c8b61182d5737..7ab995a5fcb02 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.new.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.new.js @@ -52,7 +52,6 @@ import { scheduleUpdateOnFiber, flushRoot, batchedUpdates, - unbatchedUpdates, flushSync, flushControlled, deferredUpdates, @@ -327,7 +326,6 @@ export function updateContainer( export { batchedUpdates, - unbatchedUpdates, deferredUpdates, discreteUpdates, flushControlled, diff --git a/packages/react-reconciler/src/ReactFiberReconciler.old.js b/packages/react-reconciler/src/ReactFiberReconciler.old.js index 7e74bf6a1feea..da127689c7c6e 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.old.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.old.js @@ -52,7 +52,6 @@ import { scheduleUpdateOnFiber, flushRoot, batchedUpdates, - unbatchedUpdates, flushSync, flushControlled, deferredUpdates, @@ -327,7 +326,6 @@ export function updateContainer( export { batchedUpdates, - unbatchedUpdates, deferredUpdates, discreteUpdates, flushControlled, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index c8e13fa3a5eb2..aa36d6da831e7 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -246,12 +246,11 @@ const { type ExecutionContext = number; -export const NoContext = /* */ 0b00000; -const BatchedContext = /* */ 0b00001; -const LegacyUnbatchedContext = /* */ 0b00010; -const RenderContext = /* */ 0b00100; -const CommitContext = /* */ 0b01000; -export const RetryAfterError = /* */ 0b10000; +export const NoContext = /* */ 0b0000; +const BatchedContext = /* */ 0b0001; +const RenderContext = /* */ 0b0010; +const CommitContext = /* */ 0b0100; +export const RetryAfterError = /* */ 0b1000; type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5; const RootIncomplete = 0; @@ -515,35 +514,19 @@ export function scheduleUpdateOnFiber( } } - if (lane === SyncLane) { - if ( - // Check if we're inside unbatchedUpdates - (executionContext & LegacyUnbatchedContext) !== NoContext && - // Check if we're not already rendering - (executionContext & (RenderContext | CommitContext)) === NoContext - ) { - // This is a legacy edge case. The initial mount of a ReactDOM.render-ed - // root inside of batchedUpdates should be synchronous, but layout updates - // should be deferred until the end of the batch. - performSyncWorkOnRoot(root); - } else { - ensureRootIsScheduled(root, eventTime); - if ( - executionContext === NoContext && - (fiber.mode & ConcurrentMode) === NoMode - ) { - // Flush the synchronous work now, unless we're already working or inside - // a batch. This is intentionally inside scheduleUpdateOnFiber instead of - // scheduleCallbackForFiber to preserve the ability to schedule a callback - // without immediately flushing it. We only do this for user-initiated - // updates, to preserve historical behavior of legacy mode. - resetRenderTimer(); - flushSyncCallbacksOnlyInLegacyMode(); - } - } - } else { - // Schedule other updates after in case the callback is sync. - ensureRootIsScheduled(root, eventTime); + ensureRootIsScheduled(root, eventTime); + if ( + lane === SyncLane && + executionContext === NoContext && + (fiber.mode & ConcurrentMode) === NoMode + ) { + // Flush the synchronous work now, unless we're already working or inside + // a batch. This is intentionally inside scheduleUpdateOnFiber instead of + // scheduleCallbackForFiber to preserve the ability to schedule a callback + // without immediately flushing it. We only do this for user-initiated + // updates, to preserve historical behavior of legacy mode. + resetRenderTimer(); + flushSyncCallbacksOnlyInLegacyMode(); } return root; @@ -1095,25 +1078,6 @@ export function discreteUpdates( } } -export function unbatchedUpdates(fn: (a: A) => R, a: A): R { - const prevExecutionContext = executionContext; - executionContext &= ~BatchedContext; - executionContext |= LegacyUnbatchedContext; - 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) { - resetRenderTimer(); - // TODO: I think this call is redundant, because we flush inside - // scheduleUpdateOnFiber when LegacyUnbatchedContext is set. - flushSyncCallbacksOnlyInLegacyMode(); - } - } -} - export function flushSyncWithoutWarningIfAlreadyRendering( fn: A => R, a: A, @@ -1954,24 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) { throw error; } - if ((executionContext & LegacyUnbatchedContext) !== NoContext) { - if (__DEV__) { - if (enableDebugTracing) { - logCommitStopped(); - } - } - - if (enableSchedulingProfiler) { - markCommitStopped(); - } - - // This is a legacy edge case. We just committed the initial mount of - // a ReactDOM.render-ed root inside of batchedUpdates. The commit fired - // synchronously, but layout updates should be deferred until the end - // of the batch. - return null; - } - // If the passive effects are the result of a discrete render, flush them // synchronously at the end of the current task so that the result is // immediately observable. Otherwise, we assume that they are not diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index c4f51481eaf98..2fd46c7cda7ca 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -246,12 +246,11 @@ const { type ExecutionContext = number; -export const NoContext = /* */ 0b00000; -const BatchedContext = /* */ 0b00001; -const LegacyUnbatchedContext = /* */ 0b00010; -const RenderContext = /* */ 0b00100; -const CommitContext = /* */ 0b01000; -export const RetryAfterError = /* */ 0b10000; +export const NoContext = /* */ 0b0000; +const BatchedContext = /* */ 0b0001; +const RenderContext = /* */ 0b0010; +const CommitContext = /* */ 0b0100; +export const RetryAfterError = /* */ 0b1000; type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5; const RootIncomplete = 0; @@ -515,35 +514,19 @@ export function scheduleUpdateOnFiber( } } - if (lane === SyncLane) { - if ( - // Check if we're inside unbatchedUpdates - (executionContext & LegacyUnbatchedContext) !== NoContext && - // Check if we're not already rendering - (executionContext & (RenderContext | CommitContext)) === NoContext - ) { - // This is a legacy edge case. The initial mount of a ReactDOM.render-ed - // root inside of batchedUpdates should be synchronous, but layout updates - // should be deferred until the end of the batch. - performSyncWorkOnRoot(root); - } else { - ensureRootIsScheduled(root, eventTime); - if ( - executionContext === NoContext && - (fiber.mode & ConcurrentMode) === NoMode - ) { - // Flush the synchronous work now, unless we're already working or inside - // a batch. This is intentionally inside scheduleUpdateOnFiber instead of - // scheduleCallbackForFiber to preserve the ability to schedule a callback - // without immediately flushing it. We only do this for user-initiated - // updates, to preserve historical behavior of legacy mode. - resetRenderTimer(); - flushSyncCallbacksOnlyInLegacyMode(); - } - } - } else { - // Schedule other updates after in case the callback is sync. - ensureRootIsScheduled(root, eventTime); + ensureRootIsScheduled(root, eventTime); + if ( + lane === SyncLane && + executionContext === NoContext && + (fiber.mode & ConcurrentMode) === NoMode + ) { + // Flush the synchronous work now, unless we're already working or inside + // a batch. This is intentionally inside scheduleUpdateOnFiber instead of + // scheduleCallbackForFiber to preserve the ability to schedule a callback + // without immediately flushing it. We only do this for user-initiated + // updates, to preserve historical behavior of legacy mode. + resetRenderTimer(); + flushSyncCallbacksOnlyInLegacyMode(); } return root; @@ -1095,25 +1078,6 @@ export function discreteUpdates( } } -export function unbatchedUpdates(fn: (a: A) => R, a: A): R { - const prevExecutionContext = executionContext; - executionContext &= ~BatchedContext; - executionContext |= LegacyUnbatchedContext; - 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) { - resetRenderTimer(); - // TODO: I think this call is redundant, because we flush inside - // scheduleUpdateOnFiber when LegacyUnbatchedContext is set. - flushSyncCallbacksOnlyInLegacyMode(); - } - } -} - export function flushSyncWithoutWarningIfAlreadyRendering( fn: A => R, a: A, @@ -1954,24 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) { throw error; } - if ((executionContext & LegacyUnbatchedContext) !== NoContext) { - if (__DEV__) { - if (enableDebugTracing) { - logCommitStopped(); - } - } - - if (enableSchedulingProfiler) { - markCommitStopped(); - } - - // This is a legacy edge case. We just committed the initial mount of - // a ReactDOM.render-ed root inside of batchedUpdates. The commit fired - // synchronously, but layout updates should be deferred until the end - // of the batch. - return null; - } - // If the passive effects are the result of a discrete render, flush them // synchronously at the end of the current task so that the result is // immediately observable. Otherwise, we assume that they are not diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.js index b603b03c696f5..9c94900851475 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.js @@ -349,63 +349,4 @@ describe('ReactIncrementalScheduling', () => { // The updates should all be flushed with Task priority expect(ReactNoop).toMatchRenderedOutput(); }); - - it('can opt-out of batching using unbatchedUpdates', () => { - ReactNoop.flushSync(() => { - ReactNoop.render(); - expect(ReactNoop.getChildren()).toEqual([]); - // Should not have flushed yet because we're still batching - - // unbatchedUpdates reverses the effect of batchedUpdates, so sync - // updates are not batched - ReactNoop.unbatchedUpdates(() => { - ReactNoop.render(); - expect(ReactNoop).toMatchRenderedOutput(); - ReactNoop.render(); - expect(ReactNoop).toMatchRenderedOutput(); - }); - - ReactNoop.render(); - expect(ReactNoop).toMatchRenderedOutput(); - }); - // Remaining update is now flushed - expect(ReactNoop).toMatchRenderedOutput(); - }); - - it('nested updates are always deferred, even inside unbatchedUpdates', () => { - let instance; - class Foo extends React.Component { - state = {step: 0}; - componentDidUpdate() { - Scheduler.unstable_yieldValue('componentDidUpdate: ' + this.state.step); - if (this.state.step === 1) { - ReactNoop.unbatchedUpdates(() => { - // This is a nested state update, so it should not be - // flushed synchronously, even though we wrapped it - // in unbatchedUpdates. - this.setState({step: 2}); - }); - expect(Scheduler).toHaveYielded([ - 'render: 1', - 'componentDidUpdate: 1', - ]); - expect(ReactNoop).toMatchRenderedOutput(); - } - } - render() { - Scheduler.unstable_yieldValue('render: ' + this.state.step); - instance = this; - return ; - } - } - ReactNoop.render(); - expect(Scheduler).toFlushAndYield(['render: 0']); - expect(ReactNoop).toMatchRenderedOutput(); - - ReactNoop.flushSync(() => { - instance.setState({step: 1}); - }); - expect(Scheduler).toHaveYielded(['render: 2', 'componentDidUpdate: 2']); - expect(ReactNoop).toMatchRenderedOutput(); - }); });