Make sure deletions don't stop passive effects

Before the fix, the passive effect in the test is never executed.

We were previously waiting until the next commit phase to run effects. Now, we run them before scheduling work.

packages/react-reconciler/src/ReactFiberClassComponent.js

@@ -51,6 +51,7 @@ import {
   requestCurrentTime,
   computeExpirationForFiber,
   scheduleWork,
+  flushPassiveEffectsBeforeSchedulingUpdateOnFiber,
 } from './ReactFiberScheduler';
 
 const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
@@ -199,6 +200,7 @@ const classComponentUpdater = {
       update.callback = callback;
     }
 
+    flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber);
     enqueueUpdate(fiber, update);
     scheduleWork(fiber, expirationTime);
   },
@@ -218,6 +220,7 @@ const classComponentUpdater = {
       update.callback = callback;
     }
 
+    flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber);
     enqueueUpdate(fiber, update);
     scheduleWork(fiber, expirationTime);
   },
@@ -236,6 +239,7 @@ const classComponentUpdater = {
       update.callback = callback;
     }
 
+    flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber);
     enqueueUpdate(fiber, update);
     scheduleWork(fiber, expirationTime);
   },

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -83,6 +83,7 @@ import {
 } from './ReactFiberHostConfig';
 import {
   captureCommitPhaseError,
+  flushPassiveEffectsBeforeSchedulingUpdateOnFiber,
   requestCurrentTime,
   scheduleWork,
 } from './ReactFiberScheduler';
@@ -452,6 +453,7 @@ function commitLifeCycles(
           timedOutAt: NoWork,
         };
         finishedWork.memoizedState = newState;
+        flushPassiveEffectsBeforeSchedulingUpdateOnFiber(finishedWork);
         scheduleWork(finishedWork, Sync);
         return;
       }

packages/react-reconciler/src/ReactFiberHooks.js

@@ -32,6 +32,7 @@ import {
 import {
   scheduleWork,
   computeExpirationForFiber,
+  flushPassiveEffectsBeforeSchedulingUpdateOnFiber,
   requestCurrentTime,
 } from './ReactFiberScheduler';
 
@@ -724,6 +725,7 @@ function dispatchAction<S, A>(
       callback: callback !== undefined ? callback : null,
       next: null,
     };
+    flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber);
     // Append the update to the end of the list.
     const last = queue.last;
     if (last === null) {

packages/react-reconciler/src/ReactFiberReconciler.js

@@ -52,6 +52,7 @@ import {
   syncUpdates,
   interactiveUpdates,
   flushInteractiveUpdates,
+  flushPassiveEffectsBeforeSchedulingUpdateOnFiber,
 } from './ReactFiberScheduler';
 import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
 import ReactFiberInstrumentation from './ReactFiberInstrumentation';
@@ -145,9 +146,11 @@ function scheduleRootUpdate(
     );
     update.callback = callback;
   }
-  enqueueUpdate(current, update);
 
+  flushPassiveEffectsBeforeSchedulingUpdateOnFiber(current);
+  enqueueUpdate(current, update);
   scheduleWork(current, expirationTime);
+
   return expirationTime;
 }
 

packages/react-reconciler/src/ReactFiberRoot.js

@@ -36,9 +36,6 @@ type BaseFiberRootProperties = {|
   // The currently active root fiber. This is the mutable root of the tree.
   current: Fiber,
 
-  serialEffectCallback: (() => mixed) | null,
-  serialEffectCallbackHandle: any,
-
   // The following priority levels are used to distinguish between 1)
   // uncommitted work, 2) uncommitted work that is suspended, and 3) uncommitted
   // work that may be unsuspended. We choose not to track each individual
@@ -117,8 +114,6 @@ export function createFiberRoot(
       current: uninitializedFiber,
       containerInfo: containerInfo,
       pendingChildren: null,
-      serialEffectCallback: null,
-      serialEffectCallbackHandle: null,
 
       earliestPendingTime: NoWork,
       latestPendingTime: NoWork,
@@ -148,8 +143,6 @@ export function createFiberRoot(
       current: uninitializedFiber,
       containerInfo: containerInfo,
       pendingChildren: null,
-      serialEffectCallback: null,
-      serialEffectCallbackHandle: null,
 
       earliestPendingTime: NoWork,
       latestPendingTime: NoWork,

packages/react-reconciler/src/ReactFiberScheduler.js

@@ -264,7 +264,9 @@ let nextRenderDidError: boolean = false;
 let nextEffect: Fiber | null = null;
 
 let isCommitting: boolean = false;
-let needsPassiveCommit: boolean = false;
+let rootWithPendingPassiveEffects: FiberRoot | null = null;
+let firstPassiveEffect: Fiber | null = null;
+let passiveEffectCallbackHandle: * = null;
 
 let legacyErrorBoundariesThatAlreadyFailed: Set<mixed> | null = null;
 
@@ -504,7 +506,7 @@ function commitAllLifeCycles(
     }
 
     if (effectTag & Passive) {
-      needsPassiveCommit = true;
+      rootWithPendingPassiveEffects = finishedRoot;
     }
 
     nextEffect = nextEffect.nextEffect;
@@ -512,6 +514,9 @@ function commitAllLifeCycles(
 }
 
 function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {
+  rootWithPendingPassiveEffects = null;
+  passiveEffectCallbackHandle = null;
+
   // Set this to true to prevent re-entrancy
   const previousIsRendering = isRendering;
   isRendering = true;
@@ -566,21 +571,42 @@ function markLegacyErrorBoundaryAsFailed(instance: mixed) {
   }
 }
 
-function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
-  const existingSerialEffectCallback = root.serialEffectCallback;
-  const existingSerialEffectCallbackHandle = root.serialEffectCallbackHandle;
-  if (existingSerialEffectCallback !== null) {
-    // A passive callback was scheduled during the previous commit, but it did
-    // not get a chance to flush. Flush it now to ensure serial execution.
-    // This should fire before any new mutations.
-    root.serialEffectCallback = null;
-    if (existingSerialEffectCallbackHandle !== null) {
-      root.serialEffectCallbackHandle = null;
-      Schedule_cancelCallback(existingSerialEffectCallbackHandle);
-    }
-    existingSerialEffectCallback();
+function flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber: Fiber) {
+  if (rootWithPendingPassiveEffects !== null) {
+    // TODO: This is an unfortunate extra loop. We end up traversing to the root
+    // again in scheduleWorkToRoot. But we have to do this one first because it
+    // needs to happen before adding an update to the queue, and
+    // scheduleWorkToRoot may perform a synchronous re-render. Maybe we can
+    // solve this with batchedUpdates, or with the equivalent in the Scheduler
+    // package.
+    let node = fiber;
+    do {
+      switch (node.tag) {
+        case HostRoot: {
+          const root: FiberRoot = node.stateNode;
+          flushPassiveEffects(root);
+          return;
+        }
+      }
+      node = node.return;
+    } while (node !== null);
   }
+}
 
+function flushPassiveEffects(root: FiberRoot) {
+  if (root === rootWithPendingPassiveEffects) {
+    rootWithPendingPassiveEffects = null;
+    if (passiveEffectCallbackHandle !== null) {
+      Schedule_cancelCallback(passiveEffectCallbackHandle);
+      passiveEffectCallbackHandle = null;
+    }
+    if (firstPassiveEffect !== null) {
+      commitPassiveEffects(root, firstPassiveEffect);
+    }
+  }
+}
+
+function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
   isWorking = true;
   isCommitting = true;
   startCommitTimer();
@@ -770,31 +796,30 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
     }
   }
 
-  if (firstEffect !== null && needsPassiveCommit) {
-    const resolvedFirstEffect = firstEffect;
+  if (firstEffect !== null && rootWithPendingPassiveEffects !== null) {
     // This commit included a passive effect. These do not need to fire until
     // after the next paint. Schedule an callback to fire them in an async
     // event. To ensure serial execution, the callback will be flushed early if
-    // we enter another commit phase before then.
-    needsPassiveCommit = false;
-    let serialEffectCallback;
+    // we enter rootWithPendingPassiveEffects commit phase before then.
+    const resolvedFirstEffect = (firstPassiveEffect = firstEffect);
+
+    let passiveEffectCallback;
     if (enableSchedulerTracing) {
       // TODO: Avoid this extra callback by mutating the tracing ref directly,
       // like we do at the beginning of commitRoot. I've opted not to do that
       // here because that code is still in flux.
-      serialEffectCallback = Schedule_tracing_wrap(() => {
-        root.serialEffectCallback = null;
+      passiveEffectCallback = Schedule_tracing_wrap(() => {
         commitPassiveEffects(root, resolvedFirstEffect);
       });
     } else {
-      serialEffectCallback = () => {
-        root.serialEffectCallback = null;
-        commitPassiveEffects(root, resolvedFirstEffect);
-      };
+      passiveEffectCallback = commitPassiveEffects.bind(
+        null,
+        root,
+        resolvedFirstEffect,
+      );
     }
-    root.serialEffectCallback = serialEffectCallback;
-    root.serialEffectCallbackHandle = Schedule_scheduleCallback(
-      serialEffectCallback,
+    passiveEffectCallbackHandle = Schedule_scheduleCallback(
+      passiveEffectCallback,
     );
   }
 
@@ -1227,6 +1252,9 @@ function renderRoot(
     'renderRoot was called recursively. This error is likely caused ' +
       'by a bug in React. Please file an issue.',
   );
+
+  flushPassiveEffects(root);
+
   isWorking = true;
   ReactCurrentOwner.currentDispatcher = Dispatcher;
 
@@ -1505,11 +1533,8 @@ function renderRoot(
   onComplete(root, rootWorkInProgress, expirationTime);
 }
 
-function dispatch(
-  sourceFiber: Fiber,
-  value: mixed,
-  expirationTime: ExpirationTime,
-) {
+function captureCommitPhaseError(sourceFiber: Fiber, value: mixed) {
+  const expirationTime = Sync;
   let fiber = sourceFiber.return;
   while (fiber !== null) {
     switch (fiber.tag) {
@@ -1554,10 +1579,6 @@ function dispatch(
   }
 }
 
-function captureCommitPhaseError(fiber: Fiber, error: mixed) {
-  return dispatch(fiber, error, Sync);
-}
-
 function computeThreadID(
   expirationTime: ExpirationTime,
   interactionThreadID: number,
@@ -2587,4 +2608,5 @@ export {
   interactiveUpdates,
   flushInteractiveUpdates,
   computeUniqueAsyncExpiration,
+  flushPassiveEffectsBeforeSchedulingUpdateOnFiber,
 };

packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js

@@ -604,6 +604,38 @@ describe('ReactHooks', () => {
       expect(ReactNoop.clearYields()).toEqual(['Did commit [1]']);
     });
 
+    it('flushes passive effects even with sibling deletions', () => {
+      function LayoutEffect(props) {
+        useLayoutEffect(() => {
+          ReactNoop.yield(`Layout effect`);
+        });
+        return <Text text="Layout" />;
+      }
+      function PassiveEffect(props) {
+        useEffect(() => {
+          ReactNoop.yield(`Passive effect`);
+        }, []);
+        return <Text text="Passive" />;
+      }
+      let passive = <PassiveEffect key="p" />;
+      ReactNoop.render([<LayoutEffect key="l" />, passive]);
+      expect(ReactNoop.flush()).toEqual(['Layout', 'Passive', 'Layout effect']);
+      expect(ReactNoop.getChildren()).toEqual([
+        span('Layout'),
+        span('Passive'),
+      ]);
+
+      // Destroying the first child shouldn't prevent the passive effect from
+      // being executed
+      ReactNoop.render([passive]);
+      expect(ReactNoop.flush()).toEqual(['Passive effect']);
+      expect(ReactNoop.getChildren()).toEqual([span('Passive')]);
+
+      // (No effects are left to flush.)
+      flushPassiveEffects();
+      expect(ReactNoop.clearYields()).toEqual(null);
+    });
+
     it(
       'flushes effects serially by flushing old effects before flushing ' +
         "new ones, if they haven't already fired",
@@ -631,9 +663,9 @@ describe('ReactHooks', () => {
         // Before the effects have a chance to flush, schedule another update
         ReactNoop.render(<Counter count={1} />);
         expect(ReactNoop.flush()).toEqual([
-          1,
-          // The previous effect flushes before the host mutations
+          // The previous effect flushes before the reconciliation
           'Committed state when effect was fired: 0',
+          1,
         ]);
         expect(ReactNoop.getChildren()).toEqual([span(1)]);
 
@@ -689,17 +721,39 @@ describe('ReactHooks', () => {
 
       // Rendering again should flush the previous commit's effects
       ReactNoop.render(<Counter count={1} />);
-      ReactNoop.flushThrough([
-        'Count: (empty)',
-        'Schedule update [0]',
-        'Count: 0',
-      ]);
+      ReactNoop.flushThrough(['Schedule update [0]', 'Count: 0']);
       expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);
 
+      expect(ReactNoop.flush()).toEqual([]);
+      expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
+
+      flushPassiveEffects();
       expect(ReactNoop.flush()).toEqual(['Schedule update [1]', 'Count: 1']);
       expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
     });
 
+    it('flushes serial effects before enqueueing work', () => {
+      let _updateCount;
+      function Counter(props) {
+        const [count, updateCount] = useState(0);
+        _updateCount = updateCount;
+        useEffect(() => {
+          ReactNoop.yield(`Will set count to 1`);
+          updateCount(1);
+        }, []);
+        return <Text text={'Count: ' + count} />;
+      }
+      ReactNoop.render(<Counter count={0} />);
+      expect(ReactNoop.flush()).toEqual(['Count: 0']);
+      expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
+
+      // Enqueuing this update forces the passive effect to be flushed --
+      // updateCount(1) happens first, so 2 wins.
+      _updateCount(2);
+      expect(ReactNoop.flush()).toEqual(['Will set count to 1', 'Count: 2']);
+      expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]);
+    });
+
     it(
       'in sync mode, useEffect is deferred and updates finish synchronously ' +
         '(in a single batch)',
@@ -1160,7 +1214,10 @@ describe('ReactHooks', () => {
       expect(committedText).toEqual('1');
 
       flushPassiveEffects();
-      expect(ReactNoop.clearYields()).toEqual(['Mount normal [current: 1]']);
+      expect(ReactNoop.clearYields()).toEqual([
+        'Unmount normal [current: 1]',
+        'Mount normal [current: 1]',
+      ]);
     });
 
     it('force flushes passive effects before firing new layout effects', () => {
@@ -1199,7 +1256,10 @@ describe('ReactHooks', () => {
       expect(committedText).toEqual('1');
 
       flushPassiveEffects();
-      expect(ReactNoop.clearYields()).toEqual(['Mount normal [current: 1]']);
+      expect(ReactNoop.clearYields()).toEqual([
+        'Unmount normal [current: 1]',
+        'Mount normal [current: 1]',
+      ]);
     });
 
     it('fires all mutation effects before firing any layout effects', () => {