Revert "Replace flushDiscreteUpdates with flushSync (facebook#21775)"

This reverts commit 32eefcb.

Author: Brian Vaughn

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.',
+        );
+      }
     }
   });
 

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(

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;
 }

packages/react-noop-renderer/src/ReactNoop.js

@@ -41,6 +41,7 @@ export const {
   unbatchedUpdates,
   discreteUpdates,
   idleUpdates,
+  flushDiscreteUpdates,
   flushSync,
   flushPassiveEffects,
   act,

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);
     },

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;

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,
 };
 

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,
 };
 

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<A>(fn: () => A): A {
   const previousPriority = getCurrentUpdatePriority();
   const prevTransition = ReactCurrentBatchConfig.transition;
@@ -1114,10 +1142,7 @@ export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {
   }
 }
 
-export function flushSyncWithoutWarningIfAlreadyRendering<A, R>(
-  fn: A => R,
-  a: A,
-): R {
+export function flushSync<A, R>(fn: A => R, a: A): R {
   const prevExecutionContext = executionContext;
   executionContext |= BatchedContext;
 
@@ -1140,23 +1165,18 @@ export function flushSyncWithoutWarningIfAlreadyRendering<A, R>(
     // 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<A, R>(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;

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<A>(fn: () => A): A {
   const previousPriority = getCurrentUpdatePriority();
   const prevTransition = ReactCurrentBatchConfig.transition;
@@ -1114,10 +1142,7 @@ export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {
   }
 }
 
-export function flushSyncWithoutWarningIfAlreadyRendering<A, R>(
-  fn: A => R,
-  a: A,
-): R {
+export function flushSync<A, R>(fn: A => R, a: A): R {
   const prevExecutionContext = executionContext;
   executionContext |= BatchedContext;
 
@@ -1140,23 +1165,18 @@ export function flushSyncWithoutWarningIfAlreadyRendering<A, R>(
     // 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<A, R>(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;

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 <Text text="Child" />;
-    }
-
-    const root = ReactNoop.createRoot();
-    await act(async () => {
-      root.render(<App />);
-      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']);
-  });
 });