Feature flag to revert facebook#15650

PR facebook#15650 is a bugfix but it's technically a semantic change that could
cause regressions. I don't think it will be an issue, since the
previous behavior was both broken and incoherent, but out of an
abundance of caution, let's wrap it in a flag so we can easily revert
it if necessary.

Author: acdlite

Diff for: packages/react-reconciler/src/ReactFiberClassComponent.js

@@ -52,6 +52,7 @@ import {
   requestCurrentTime,
   computeExpirationForFiber,
   scheduleWork,
+  flushPassiveEffects,
 } from './ReactFiberScheduler';
 
 const fakeInternalInstance = {};
@@ -193,6 +194,9 @@ const classComponentUpdater = {
       update.callback = callback;
     }
 
+    if (revertPassiveEffectsChange) {
+      flushPassiveEffects();
+    }
     enqueueUpdate(fiber, update);
     scheduleWork(fiber, expirationTime);
   },
@@ -212,6 +216,9 @@ const classComponentUpdater = {
       update.callback = callback;
     }
 
+    if (revertPassiveEffectsChange) {
+      flushPassiveEffects();
+    }
     enqueueUpdate(fiber, update);
     scheduleWork(fiber, expirationTime);
   },
@@ -230,6 +237,9 @@ const classComponentUpdater = {
       update.callback = callback;
     }
 
+    if (revertPassiveEffectsChange) {
+      flushPassiveEffects();
+    }
     enqueueUpdate(fiber, update);
     scheduleWork(fiber, expirationTime);
   },

Diff for: packages/react-reconciler/src/ReactFiberHooks.js

@@ -31,6 +31,7 @@ import {
 import {
   scheduleWork,
   computeExpirationForFiber,
+  flushPassiveEffects,
   requestCurrentTime,
   warnIfNotCurrentlyActingUpdatesInDev,
   markRenderEventTime,
@@ -41,6 +42,7 @@ import warning from 'shared/warning';
 import getComponentName from 'shared/getComponentName';
 import is from 'shared/objectIs';
 import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork';
+import {revertPassiveEffectsChange} from 'shared/ReactFeatureFlags';
 
 const {ReactCurrentDispatcher} = ReactSharedInternals;
 
@@ -1107,6 +1109,10 @@ function dispatchAction<S, A>(
       lastRenderPhaseUpdate.next = update;
     }
   } else {
+    if (revertPassiveEffectsChange) {
+      flushPassiveEffects();
+    }
+
     const currentTime = requestCurrentTime();
     const expirationTime = computeExpirationForFiber(currentTime, fiber);
 

Diff for: packages/react-reconciler/src/ReactFiberReconciler.js

@@ -64,6 +64,7 @@ import {
 } from './ReactCurrentFiber';
 import {StrictMode} from './ReactTypeOfMode';
 import {Sync} from './ReactFiberExpirationTime';
+import {revertPassiveEffectsChange} from 'shared/ReactFeatureFlags';
 
 type OpaqueRoot = FiberRoot;
 
@@ -152,6 +153,9 @@ function scheduleRootUpdate(
     update.callback = callback;
   }
 
+  if (revertPassiveEffectsChange) {
+    flushPassiveEffects();
+  }
   enqueueUpdate(current, update);
   scheduleWork(current, expirationTime);
 
@@ -391,6 +395,10 @@ if (__DEV__) {
       id--;
     }
     if (currentHook !== null) {
+      if (revertPassiveEffectChange) {
+        flushPassiveEffects();
+      }
+
       const newState = copyWithSet(currentHook.memoizedState, path, value);
       currentHook.memoizedState = newState;
       currentHook.baseState = newState;
@@ -408,6 +416,9 @@ if (__DEV__) {
 
   // Support DevTools props for function components, forwardRef, memo, host components, etc.
   overrideProps = (fiber: Fiber, path: Array<string | number>, value: any) => {
+    if (revertPassiveEffectChange) {
+      flushPassiveEffects();
+    }
     fiber.pendingProps = copyWithSet(fiber.memoizedProps, path, value);
     if (fiber.alternate) {
       fiber.alternate.pendingProps = fiber.pendingProps;
@@ -416,6 +427,9 @@ if (__DEV__) {
   };
 
   scheduleUpdate = (fiber: Fiber) => {
+    if (revertPassiveEffectChange) {
+      flushPassiveEffects();
+    }
     scheduleWork(fiber, Sync);
   };
 

Diff for: packages/react-reconciler/src/ReactFiberScheduler.js

@@ -24,6 +24,7 @@ import {
   enableProfilerTimer,
   disableYielding,
   enableSchedulerTracing,
+  revertPassiveEffectsChange,
 } from 'shared/ReactFeatureFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import invariant from 'shared/invariant';
@@ -560,9 +561,11 @@ export function flushInteractiveUpdates() {
     return;
   }
   flushPendingDiscreteUpdates();
-  // If the discrete updates scheduled passive effects, flush them now so that
-  // they fire before the next serial event.
-  flushPassiveEffects();
+  if (!revertPassiveEffectsChange) {
+    // If the discrete updates scheduled passive effects, flush them now so that
+    // they fire before the next serial event.
+    flushPassiveEffects();
+  }
 }
 
 function resolveLocksOnRoot(root: FiberRoot, expirationTime: ExpirationTime) {
@@ -598,8 +601,10 @@ export function interactiveUpdates<A, B, C, R>(
     // should explicitly call flushInteractiveUpdates.
     flushPendingDiscreteUpdates();
   }
-  // TODO: Remove this call for the same reason as above.
-  flushPassiveEffects();
+  if (!revertPassiveEffectsChange) {
+    // TODO: Remove this call for the same reason as above.
+    flushPassiveEffects();
+  }
   return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c));
 }
 
@@ -750,7 +755,9 @@ function renderRoot(
     return commitRoot.bind(null, root);
   }
 
-  flushPassiveEffects();
+  if (!revertPassiveEffectsChange) {
+    flushPassiveEffects();
+  }
 
   // If the root or expiration time have changed, throw out the existing stack
   // and prepare a fresh one. Otherwise we'll continue where we left off.

Diff for: packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js

@@ -2077,4 +2077,46 @@ describe('ReactHooksWithNoopRenderer', () => {
     expect(Scheduler).toFlushAndYield(['Step: 5, Shadow: 5']);
     expect(ReactNoop).toMatchRenderedOutput('5');
   });
+
+  describe('revertPassiveEffectsChange', () => {
+    it('flushes serial effects before enqueueing work', () => {
+      jest.resetModules();
+
+      ReactFeatureFlags = require('shared/ReactFeatureFlags');
+      ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
+      ReactFeatureFlags.enableSchedulerTracing = true;
+      ReactFeatureFlags.revertPassiveEffectsChange = true;
+      React = require('react');
+      ReactNoop = require('react-noop-renderer');
+      Scheduler = require('scheduler');
+      SchedulerTracing = require('scheduler/tracing');
+      useState = React.useState;
+      useEffect = React.useEffect;
+      act = ReactNoop.act;
+
+      let _updateCount;
+      function Counter(props) {
+        const [count, updateCount] = useState(0);
+        _updateCount = updateCount;
+        useEffect(() => {
+          Scheduler.yieldValue(`Will set count to 1`);
+          updateCount(1);
+        }, []);
+        return <Text text={'Count: ' + count} />;
+      }
+
+      ReactNoop.render(<Counter count={0} />, () =>
+        Scheduler.yieldValue('Sync effect'),
+      );
+      expect(Scheduler).toFlushAndYieldThrough(['Count: 0', 'Sync effect']);
+      expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
+
+      // Enqueuing this update forces the passive effect to be flushed --
+      // updateCount(1) happens first, so 2 wins.
+      act(() => _updateCount(2));
+      expect(Scheduler).toHaveYielded(['Will set count to 1']);
+      expect(Scheduler).toFlushAndYield(['Count: 2']);
+      expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]);
+    });
+  });
 });

Diff for: packages/shared/ReactFeatureFlags.js

@@ -67,3 +67,6 @@ export const enableEventAPI = false;
 
 // New API for JSX transforms to target - https://github.com/reactjs/rfcs/pull/107
 export const enableJSXTransformAPI = false;
+
+// Temporary flag to revert the fix in #15650
+export const revertPassiveEffectsChange = false;

Diff for: packages/shared/forks/ReactFeatureFlags.native-fb.js

@@ -32,6 +32,7 @@ export const warnAboutDeprecatedLifecycles = true;
 export const warnAboutDeprecatedSetNativeProps = true;
 export const enableEventAPI = false;
 export const enableJSXTransformAPI = false;
+export const revertPassiveEffectsChange = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {

Diff for: packages/shared/forks/ReactFeatureFlags.native-oss.js

@@ -29,6 +29,7 @@ export const enableSchedulerDebugging = false;
 export const warnAboutDeprecatedSetNativeProps = false;
 export const enableEventAPI = false;
 export const enableJSXTransformAPI = false;
+export const revertPassiveEffectsChange = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {

Diff for: packages/shared/forks/ReactFeatureFlags.persistent.js

@@ -29,6 +29,7 @@ export const enableSchedulerDebugging = false;
 export const warnAboutDeprecatedSetNativeProps = false;
 export const enableEventAPI = false;
 export const enableJSXTransformAPI = false;
+export const revertPassiveEffectsChange = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {

Diff for: packages/shared/forks/ReactFeatureFlags.test-renderer.js

@@ -29,6 +29,7 @@ export const enableSchedulerDebugging = false;
 export const warnAboutDeprecatedSetNativeProps = false;
 export const enableEventAPI = false;
 export const enableJSXTransformAPI = false;
+export const revertPassiveEffectsChange = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {

Diff for: packages/shared/forks/ReactFeatureFlags.test-renderer.www.js

@@ -27,6 +27,7 @@ export const disableJavaScriptURLs = false;
 export const disableYielding = false;
 export const enableEventAPI = true;
 export const enableJSXTransformAPI = true;
+export const revertPassiveEffectsChange = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {

Diff for: packages/shared/forks/ReactFeatureFlags.www.js

@@ -20,6 +20,7 @@ export const {
   disableInputAttributeSyncing,
   warnAboutShorthandPropertyCollision,
   warnAboutDeprecatedSetNativeProps,
+  revertPassiveEffectsChange,
 } = require('ReactFeatureFlags');
 
 // In www, we have experimental support for gathering data