From 7795b3e71c35e9be17aa7317d4eb38528c8c49e4 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 7 Jul 2021 14:59:27 -0400 Subject: [PATCH] Revert "Replace flushDiscreteUpdates with flushSync (#21775)" This reverts commit 32eefcb3c5131f4d77a2195ff11a00a2513cf62f. --- .../src/__tests__/ReactDOMFiber-test.js | 8 ++- packages/react-dom/src/client/ReactDOM.js | 4 +- .../src/events/ReactDOMUpdateBatching.js | 8 +-- packages/react-noop-renderer/src/ReactNoop.js | 1 + .../src/createReactNoop.js | 2 + .../src/ReactFiberReconciler.js | 10 ++-- .../src/ReactFiberReconciler.new.js | 4 +- .../src/ReactFiberReconciler.old.js | 4 +- .../src/ReactFiberWorkLoop.new.js | 54 +++++++++++++------ .../src/ReactFiberWorkLoop.old.js | 54 +++++++++++++------ .../src/__tests__/ReactFlushSync-test.js | 23 -------- 11 files changed, 99 insertions(+), 73 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js index ff1ea1771fc87..36c8367af5d48 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js @@ -1154,13 +1154,19 @@ describe('ReactDOMFiber', () => { expect(ops).toEqual(['A']); if (__DEV__) { - expect(console.error.calls.count()).toBe(2); + const errorCalls = console.error.calls.count(); expect(console.error.calls.argsFor(0)[0]).toMatch( 'ReactDOM.render is no longer supported in React 18', ); expect(console.error.calls.argsFor(1)[0]).toMatch( 'ReactDOM.render is no longer supported in React 18', ); + // TODO: this warning shouldn't be firing in the first place if user didn't call it. + for (let i = 2; i < errorCalls; i++) { + expect(console.error.calls.argsFor(i)[0]).toMatch( + 'unstable_flushDiscreteUpdates: Cannot flush updates when React is already rendering.', + ); + } } }); diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 80fc3d1e4a05e..8e974697075f1 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -23,8 +23,8 @@ import {createEventHandle} from './ReactDOMEventHandle'; import { batchedUpdates, discreteUpdates, + flushDiscreteUpdates, flushSync, - flushSyncWithoutWarningIfAlreadyRendering, flushControlled, injectIntoDevTools, attemptSynchronousHydration, @@ -100,7 +100,7 @@ setRestoreImplementation(restoreControlledState); setBatchingImplementation( batchedUpdates, discreteUpdates, - flushSyncWithoutWarningIfAlreadyRendering, + flushDiscreteUpdates, ); function createPortal( diff --git a/packages/react-dom/src/events/ReactDOMUpdateBatching.js b/packages/react-dom/src/events/ReactDOMUpdateBatching.js index d3c46630b148e..f2fbfe4c3f329 100644 --- a/packages/react-dom/src/events/ReactDOMUpdateBatching.js +++ b/packages/react-dom/src/events/ReactDOMUpdateBatching.js @@ -23,7 +23,7 @@ let batchedUpdatesImpl = function(fn, bookkeeping) { let discreteUpdatesImpl = function(fn, a, b, c, d) { return fn(a, b, c, d); }; -let flushSyncImpl = function() {}; +let flushDiscreteUpdatesImpl = function() {}; let isInsideEventHandler = false; @@ -39,7 +39,7 @@ function finishEventHandler() { // bails out of the update without touching the DOM. // TODO: Restore state in the microtask, after the discrete updates flush, // instead of early flushing them here. - flushSyncImpl(); + flushDiscreteUpdatesImpl(); restoreStateIfNeeded(); } } @@ -67,9 +67,9 @@ export function discreteUpdates(fn, a, b, c, d) { export function setBatchingImplementation( _batchedUpdatesImpl, _discreteUpdatesImpl, - _flushSyncImpl, + _flushDiscreteUpdatesImpl, ) { batchedUpdatesImpl = _batchedUpdatesImpl; discreteUpdatesImpl = _discreteUpdatesImpl; - flushSyncImpl = _flushSyncImpl; + flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl; } diff --git a/packages/react-noop-renderer/src/ReactNoop.js b/packages/react-noop-renderer/src/ReactNoop.js index 5e5567f9e0947..c09fa2d8000f5 100644 --- a/packages/react-noop-renderer/src/ReactNoop.js +++ b/packages/react-noop-renderer/src/ReactNoop.js @@ -41,6 +41,7 @@ export const { unbatchedUpdates, discreteUpdates, idleUpdates, + flushDiscreteUpdates, flushSync, flushPassiveEffects, act, diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index eb7e74193341c..8dbadafd2c22a 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -915,6 +915,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { } }, + flushDiscreteUpdates: NoopRenderer.flushDiscreteUpdates, + flushSync(fn: () => mixed) { NoopRenderer.flushSync(fn); }, diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index bf78e9e41c390..bc169fd8a6475 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -21,9 +21,9 @@ import { unbatchedUpdates as unbatchedUpdates_old, deferredUpdates as deferredUpdates_old, discreteUpdates as discreteUpdates_old, + flushDiscreteUpdates as flushDiscreteUpdates_old, flushControlled as flushControlled_old, flushSync as flushSync_old, - flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_old, flushPassiveEffects as flushPassiveEffects_old, getPublicRootInstance as getPublicRootInstance_old, attemptSynchronousHydration as attemptSynchronousHydration_old, @@ -59,9 +59,9 @@ import { unbatchedUpdates as unbatchedUpdates_new, deferredUpdates as deferredUpdates_new, discreteUpdates as discreteUpdates_new, + flushDiscreteUpdates as flushDiscreteUpdates_new, flushControlled as flushControlled_new, flushSync as flushSync_new, - flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_new, flushPassiveEffects as flushPassiveEffects_new, getPublicRootInstance as getPublicRootInstance_new, attemptSynchronousHydration as attemptSynchronousHydration_new, @@ -108,13 +108,13 @@ export const deferredUpdates = enableNewReconciler export const discreteUpdates = enableNewReconciler ? discreteUpdates_new : discreteUpdates_old; +export const flushDiscreteUpdates = enableNewReconciler + ? flushDiscreteUpdates_new + : flushDiscreteUpdates_old; export const flushControlled = enableNewReconciler ? flushControlled_new : flushControlled_old; export const flushSync = enableNewReconciler ? flushSync_new : flushSync_old; -export const flushSyncWithoutWarningIfAlreadyRendering = enableNewReconciler - ? flushSyncWithoutWarningIfAlreadyRendering_new - : flushSyncWithoutWarningIfAlreadyRendering_old; export const flushPassiveEffects = enableNewReconciler ? flushPassiveEffects_new : flushPassiveEffects_old; diff --git a/packages/react-reconciler/src/ReactFiberReconciler.new.js b/packages/react-reconciler/src/ReactFiberReconciler.new.js index c8b61182d5737..f406f8592b0d1 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.new.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.new.js @@ -57,7 +57,7 @@ import { flushControlled, deferredUpdates, discreteUpdates, - flushSyncWithoutWarningIfAlreadyRendering, + flushDiscreteUpdates, flushPassiveEffects, } from './ReactFiberWorkLoop.new'; import { @@ -330,9 +330,9 @@ export { unbatchedUpdates, deferredUpdates, discreteUpdates, + flushDiscreteUpdates, flushControlled, flushSync, - flushSyncWithoutWarningIfAlreadyRendering, flushPassiveEffects, }; diff --git a/packages/react-reconciler/src/ReactFiberReconciler.old.js b/packages/react-reconciler/src/ReactFiberReconciler.old.js index 7e74bf6a1feea..f6ae425b4400b 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.old.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.old.js @@ -57,7 +57,7 @@ import { flushControlled, deferredUpdates, discreteUpdates, - flushSyncWithoutWarningIfAlreadyRendering, + flushDiscreteUpdates, flushPassiveEffects, } from './ReactFiberWorkLoop.old'; import { @@ -330,9 +330,9 @@ export { unbatchedUpdates, deferredUpdates, discreteUpdates, + flushDiscreteUpdates, flushControlled, flushSync, - flushSyncWithoutWarningIfAlreadyRendering, flushPassiveEffects, }; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index c8e13fa3a5eb2..7f429c873438a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1044,6 +1044,34 @@ export function getExecutionContext(): ExecutionContext { return executionContext; } +export function flushDiscreteUpdates() { + // TODO: Should be able to flush inside batchedUpdates, but not inside `act`. + // However, `act` uses `batchedUpdates`, so there's no way to distinguish + // those two cases. Need to fix this before exposing flushDiscreteUpdates + // as a public API. + if ( + (executionContext & (BatchedContext | RenderContext | CommitContext)) !== + NoContext + ) { + if (__DEV__) { + if ((executionContext & RenderContext) !== NoContext) { + console.error( + 'unstable_flushDiscreteUpdates: Cannot flush updates when React is ' + + 'already rendering.', + ); + } + } + // We're already rendering, so we can't synchronously flush pending work. + // This is probably a nested event dispatch triggered by a lifecycle/effect, + // like `el.focus()`. Exit. + return; + } + flushSyncCallbacks(); + // If the discrete updates scheduled passive effects, flush them now so that + // they fire before the next serial event. + flushPassiveEffects(); +} + export function deferredUpdates(fn: () => A): A { const previousPriority = getCurrentUpdatePriority(); const prevTransition = ReactCurrentBatchConfig.transition; @@ -1114,10 +1142,7 @@ export function unbatchedUpdates(fn: (a: A) => R, a: A): R { } } -export function flushSyncWithoutWarningIfAlreadyRendering( - fn: A => R, - a: A, -): R { +export function flushSync(fn: A => R, a: A): R { const prevExecutionContext = executionContext; executionContext |= BatchedContext; @@ -1140,23 +1165,18 @@ export function flushSyncWithoutWarningIfAlreadyRendering( // the stack. if ((executionContext & (RenderContext | CommitContext)) === NoContext) { flushSyncCallbacks(); + } else { + if (__DEV__) { + console.error( + 'flushSync was called from inside a lifecycle method. React cannot ' + + 'flush when React is already rendering. Consider moving this call to ' + + 'a scheduler task or micro task.', + ); + } } } } -export function flushSync(fn: A => R, a: A): R { - if (__DEV__) { - if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { - console.error( - 'flushSync was called from inside a lifecycle method. React cannot ' + - 'flush when React is already rendering. Consider moving this call to ' + - 'a scheduler task or micro task.', - ); - } - } - return flushSyncWithoutWarningIfAlreadyRendering(fn, a); -} - export function flushControlled(fn: () => mixed): void { const prevExecutionContext = executionContext; executionContext |= BatchedContext; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index c4f51481eaf98..f356fc3f1156a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -1044,6 +1044,34 @@ export function getExecutionContext(): ExecutionContext { return executionContext; } +export function flushDiscreteUpdates() { + // TODO: Should be able to flush inside batchedUpdates, but not inside `act`. + // However, `act` uses `batchedUpdates`, so there's no way to distinguish + // those two cases. Need to fix this before exposing flushDiscreteUpdates + // as a public API. + if ( + (executionContext & (BatchedContext | RenderContext | CommitContext)) !== + NoContext + ) { + if (__DEV__) { + if ((executionContext & RenderContext) !== NoContext) { + console.error( + 'unstable_flushDiscreteUpdates: Cannot flush updates when React is ' + + 'already rendering.', + ); + } + } + // We're already rendering, so we can't synchronously flush pending work. + // This is probably a nested event dispatch triggered by a lifecycle/effect, + // like `el.focus()`. Exit. + return; + } + flushSyncCallbacks(); + // If the discrete updates scheduled passive effects, flush them now so that + // they fire before the next serial event. + flushPassiveEffects(); +} + export function deferredUpdates(fn: () => A): A { const previousPriority = getCurrentUpdatePriority(); const prevTransition = ReactCurrentBatchConfig.transition; @@ -1114,10 +1142,7 @@ export function unbatchedUpdates(fn: (a: A) => R, a: A): R { } } -export function flushSyncWithoutWarningIfAlreadyRendering( - fn: A => R, - a: A, -): R { +export function flushSync(fn: A => R, a: A): R { const prevExecutionContext = executionContext; executionContext |= BatchedContext; @@ -1140,23 +1165,18 @@ export function flushSyncWithoutWarningIfAlreadyRendering( // the stack. if ((executionContext & (RenderContext | CommitContext)) === NoContext) { flushSyncCallbacks(); + } else { + if (__DEV__) { + console.error( + 'flushSync was called from inside a lifecycle method. React cannot ' + + 'flush when React is already rendering. Consider moving this call to ' + + 'a scheduler task or micro task.', + ); + } } } } -export function flushSync(fn: A => R, a: A): R { - if (__DEV__) { - if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { - console.error( - 'flushSync was called from inside a lifecycle method. React cannot ' + - 'flush when React is already rendering. Consider moving this call to ' + - 'a scheduler task or micro task.', - ); - } - } - return flushSyncWithoutWarningIfAlreadyRendering(fn, a); -} - export function flushControlled(fn: () => mixed): void { const prevExecutionContext = executionContext; executionContext |= BatchedContext; diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js index c73e46ddc5e31..a3a74d739a9a3 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js @@ -173,27 +173,4 @@ describe('ReactFlushSync', () => { // Effect flushes after paint. expect(Scheduler).toHaveYielded(['Effect']); }); - - test('does not flush pending passive effects', async () => { - function App() { - useEffect(() => { - Scheduler.unstable_yieldValue('Effect'); - }, []); - return ; - } - - const root = ReactNoop.createRoot(); - await act(async () => { - root.render(); - expect(Scheduler).toFlushUntilNextPaint(['Child']); - expect(root).toMatchRenderedOutput('Child'); - - // Passive effects are pending. Calling flushSync should not affect them. - ReactNoop.flushSync(); - // Effects still haven't fired. - expect(Scheduler).toHaveYielded([]); - }); - // Now the effects have fired. - expect(Scheduler).toHaveYielded(['Effect']); - }); });