Replace flushDiscreteUpdates with flushSync

flushDiscreteUpdates is almost the same as flushSync. It forces passive
effects to fire, because of an outdated heuristic, which isn't ideal but
not that important.

Besides that, the only remaining difference between flushDiscreteUpdates
and flushSync is that flushDiscreteUpdates does not warn if you call it
from inside an effect/lifecycle. This is because it might get triggered
by a nested event dispatch, like `el.focus()`.

So I added a new method, flushSyncWithWarningIfAlreadyRendering, which
is used for the public flushSync API. It includes the warning. And I
removed the warning from flushSync, so the event system can call that
one. In production, flushSyncWithWarningIfAlreadyRendering gets inlined
to flushSync, so the behavior is identical.

Another way of thinking about this PR is that I renamed flushSync to
flushSyncWithWarningIfAlreadyRendering and flushDiscreteUpdates to
flushSync (and fixed the passive effects thing). The point is to prevent
these from subtly diverging in the future.

Author: acdlite

packages/react-dom/src/__tests__/ReactDOMFiber-test.js

@@ -1154,19 +1154,13 @@ describe('ReactDOMFiber', () => {
     expect(ops).toEqual(['A']);
 
     if (__DEV__) {
-      const errorCalls = console.error.calls.count();
+      expect(console.error.calls.count()).toBe(2);
       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,
+  flushSyncWithWarningIfAlreadyRendering,
   flushControlled,
   injectIntoDevTools,
   attemptSynchronousHydration,
@@ -97,11 +97,7 @@ if (__DEV__) {
 }
 
 setRestoreImplementation(restoreControlledState);
-setBatchingImplementation(
-  batchedUpdates,
-  discreteUpdates,
-  flushDiscreteUpdates,
-);
+setBatchingImplementation(batchedUpdates, discreteUpdates, flushSync);
 
 function createPortal(
   children: ReactNodeList,
@@ -166,7 +162,7 @@ const Internals = {
 export {
   createPortal,
   batchedUpdates as unstable_batchedUpdates,
-  flushSync,
+  flushSyncWithWarningIfAlreadyRendering as flushSync,
   Internals as __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED,
   ReactVersion as version,
   // Disabled behind disableLegacyReactDOMAPIs

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 flushDiscreteUpdatesImpl = function() {};
+let flushSyncImpl = 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.
-    flushDiscreteUpdatesImpl();
+    flushSyncImpl();
     restoreStateIfNeeded();
   }
 }
@@ -67,9 +67,9 @@ export function discreteUpdates(fn, a, b, c, d) {
 export function setBatchingImplementation(
   _batchedUpdatesImpl,
   _discreteUpdatesImpl,
-  _flushDiscreteUpdatesImpl,
+  _flushSyncImpl,
 ) {
   batchedUpdatesImpl = _batchedUpdatesImpl;
   discreteUpdatesImpl = _discreteUpdatesImpl;
-  flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl;
+  flushSyncImpl = _flushSyncImpl;
 }

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

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

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

@@ -915,10 +915,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
       }
     },
 
-    flushDiscreteUpdates: NoopRenderer.flushDiscreteUpdates,
-
     flushSync(fn: () => mixed) {
-      NoopRenderer.flushSync(fn);
+      NoopRenderer.flushSyncWithWarningIfAlreadyRendering(fn);
     },
 
     flushPassiveEffects: NoopRenderer.flushPassiveEffects,

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,
+  flushSyncWithWarningIfAlreadyRendering as flushSyncWithWarningIfAlreadyRendering_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,
+  flushSyncWithWarningIfAlreadyRendering as flushSyncWithWarningIfAlreadyRendering_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 flushSyncWithWarningIfAlreadyRendering = enableNewReconciler
+  ? flushSyncWithWarningIfAlreadyRendering_new
+  : flushSyncWithWarningIfAlreadyRendering_old;
 export const flushPassiveEffects = enableNewReconciler
   ? flushPassiveEffects_new
   : flushPassiveEffects_old;

packages/react-reconciler/src/ReactFiberReconciler.new.js

@@ -57,7 +57,7 @@ import {
   flushControlled,
   deferredUpdates,
   discreteUpdates,
-  flushDiscreteUpdates,
+  flushSyncWithWarningIfAlreadyRendering,
   flushPassiveEffects,
 } from './ReactFiberWorkLoop.new';
 import {
@@ -330,9 +330,9 @@ export {
   unbatchedUpdates,
   deferredUpdates,
   discreteUpdates,
-  flushDiscreteUpdates,
   flushControlled,
   flushSync,
+  flushSyncWithWarningIfAlreadyRendering,
   flushPassiveEffects,
 };
 

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

@@ -1044,34 +1044,6 @@ 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;
@@ -1165,18 +1137,26 @@ export function flushSync<A, R>(fn: A => R, a: 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 flushSyncWithWarningIfAlreadyRendering<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 flushSync(fn, a);
+}
+
 export function flushControlled(fn: () => mixed): void {
   const prevExecutionContext = executionContext;
   executionContext |= BatchedContext;

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

@@ -1044,34 +1044,6 @@ 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;
@@ -1165,18 +1137,26 @@ export function flushSync<A, R>(fn: A => R, a: 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 flushSyncWithWarningIfAlreadyRendering<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 flushSync(fn, a);
+}
+
 export function flushControlled(fn: () => mixed): void {
   const prevExecutionContext = executionContext;
   executionContext |= BatchedContext;

packages/react-reconciler/src/__tests__/ReactFlushSync-test.js

@@ -173,4 +173,27 @@ 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']);
+  });
 });