Fix serial passive effects (#15650)

* Failing test for false positive warning

* Flush passive effects before discrete events

Currently, we check for pending passive effects inside the `setState`
method before we add additional updates to the queue, in case those
pending effects also add things to the queue.

However, the `setState` method is too late, because the event that
caused the update might not have ever fired had the passive effects
flushed before we got there.

This is the same as the discrete/serial events problem. When a serial
update comes in, and there's already a pending serial update, we have to
do it before we call the user-provided event handlers. Because the event
handlers themselves might change as a result of the pending update.

This commit moves the `flushPassiveEffects` call to before the discrete
event handlers are called, and removes it from the `setState` method.
Non-discrete events will not cause passive effects to flush, which is
fine, since by definition they are not order dependent.

Author: acdlite

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

@@ -72,42 +72,6 @@ describe('ReactDOMHooks', () => {
     expect(container3.textContent).toBe('6');
   });
 
-  it('can batch synchronous work inside effects with other work', () => {
-    let otherContainer = document.createElement('div');
-
-    let calledA = false;
-    function A() {
-      calledA = true;
-      return 'A';
-    }
-
-    let calledB = false;
-    function B() {
-      calledB = true;
-      return 'B';
-    }
-
-    let _set;
-    function Foo() {
-      _set = React.useState(0)[1];
-      React.useEffect(() => {
-        ReactDOM.render(<A />, otherContainer);
-      });
-      return null;
-    }
-
-    ReactDOM.render(<Foo />, container);
-    ReactDOM.unstable_batchedUpdates(() => {
-      _set(0); // Forces the effect to be flushed
-      expect(otherContainer.textContent).toBe('A');
-      ReactDOM.render(<B />, otherContainer);
-      expect(otherContainer.textContent).toBe('A');
-    });
-    expect(otherContainer.textContent).toBe('B');
-    expect(calledA).toBe(true);
-    expect(calledB).toBe(true);
-  });
-
   it('should not bail out when an update is scheduled from within an event handler', () => {
     const {createRef, useCallback, useState} = React;
 

packages/react-reconciler/src/ReactFiberClassComponent.js

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

packages/react-reconciler/src/ReactFiberHooks.js

@@ -31,7 +31,6 @@ import {
 import {
   scheduleWork,
   computeExpirationForFiber,
-  flushPassiveEffects,
   requestCurrentTime,
   warnIfNotCurrentlyActingUpdatesInDev,
   markRenderEventTime,
@@ -1108,8 +1107,6 @@ function dispatchAction<S, A>(
       lastRenderPhaseUpdate.next = update;
     }
   } else {
-    flushPassiveEffects();
-
     const currentTime = requestCurrentTime();
     const expirationTime = computeExpirationForFiber(currentTime, fiber);
 

packages/react-reconciler/src/ReactFiberReconciler.js

@@ -152,7 +152,6 @@ function scheduleRootUpdate(
     update.callback = callback;
   }
 
-  flushPassiveEffects();
   enqueueUpdate(current, update);
   scheduleWork(current, expirationTime);
 
@@ -392,8 +391,6 @@ if (__DEV__) {
       id--;
     }
     if (currentHook !== null) {
-      flushPassiveEffects();
-
       const newState = copyWithSet(currentHook.memoizedState, path, value);
       currentHook.memoizedState = newState;
       currentHook.baseState = newState;
@@ -411,7 +408,6 @@ if (__DEV__) {
 
   // Support DevTools props for function components, forwardRef, memo, host components, etc.
   overrideProps = (fiber: Fiber, path: Array<string | number>, value: any) => {
-    flushPassiveEffects();
     fiber.pendingProps = copyWithSet(fiber.memoizedProps, path, value);
     if (fiber.alternate) {
       fiber.alternate.pendingProps = fiber.pendingProps;
@@ -420,7 +416,6 @@ if (__DEV__) {
   };
 
   scheduleUpdate = (fiber: Fiber) => {
-    flushPassiveEffects();
     scheduleWork(fiber, Sync);
   };
 

packages/react-reconciler/src/ReactFiberScheduler.js

@@ -560,6 +560,9 @@ 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();
 }
 
 function resolveLocksOnRoot(root: FiberRoot, expirationTime: ExpirationTime) {
@@ -595,6 +598,8 @@ export function interactiveUpdates<A, B, C, R>(
     // should explicitly call flushInteractiveUpdates.
     flushPendingDiscreteUpdates();
   }
+  // TODO: Remove this call for the same reason as above.
+  flushPassiveEffects();
   return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c));
 }
 

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

@@ -1719,6 +1719,58 @@ describe('ReactHooks', () => {
     ).toThrow('Hello');
   });
 
+  // Regression test for https://github.com/facebook/react/issues/15057
+  it('does not fire a false positive warning when previous effect unmounts the component', () => {
+    let {useState, useEffect} = React;
+    let globalListener;
+
+    function A() {
+      const [show, setShow] = useState(true);
+      function hideMe() {
+        setShow(false);
+      }
+      return show ? <B hideMe={hideMe} /> : null;
+    }
+
+    function B(props) {
+      return <C {...props} />;
+    }
+
+    function C({hideMe}) {
+      const [, setState] = useState();
+
+      useEffect(() => {
+        let isStale = false;
+
+        globalListener = () => {
+          if (!isStale) {
+            setState('hello');
+          }
+        };
+
+        return () => {
+          isStale = true;
+          hideMe();
+        };
+      });
+      return null;
+    }
+
+    ReactTestRenderer.act(() => {
+      ReactTestRenderer.create(<A />);
+    });
+
+    expect(() => {
+      globalListener();
+      globalListener();
+    }).toWarnDev([
+      'An update to C inside a test was not wrapped in act',
+      'An update to C inside a test was not wrapped in act',
+      // Note: should *not* warn about updates on unmounted component.
+      // Because there's no way for component to know it got unmounted.
+    ]);
+  });
+
   // Regression test for https://github.com/facebook/react/issues/14790
   it('does not fire a false positive warning when suspending memo', async () => {
     const {Suspense, useState} = React;

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

@@ -670,8 +670,7 @@ describe('ReactHooksWithNoopRenderer', () => {
       // Destroying the first child shouldn't prevent the passive effect from
       // being executed
       ReactNoop.render([passive]);
-      expect(Scheduler).toHaveYielded(['Passive effect']);
-      expect(Scheduler).toFlushAndYield([]);
+      expect(Scheduler).toFlushAndYield(['Passive effect']);
       expect(ReactNoop.getChildren()).toEqual([span('Passive')]);
 
       // (No effects are left to flush.)
@@ -776,11 +775,12 @@ describe('ReactHooksWithNoopRenderer', () => {
         ReactNoop.render(<Counter count={1} />, () =>
           Scheduler.yieldValue('Sync effect'),
         );
-        expect(Scheduler).toHaveYielded([
+        expect(Scheduler).toFlushAndYieldThrough([
           // The previous effect flushes before the reconciliation
           'Committed state when effect was fired: 0',
+          1,
+          'Sync effect',
         ]);
-        expect(Scheduler).toFlushAndYieldThrough([1, 'Sync effect']);
         expect(ReactNoop.getChildren()).toEqual([span(1)]);
 
         ReactNoop.flushPassiveEffects();
@@ -849,8 +849,10 @@ describe('ReactHooksWithNoopRenderer', () => {
       ReactNoop.render(<Counter count={1} />, () =>
         Scheduler.yieldValue('Sync effect'),
       );
-      expect(Scheduler).toHaveYielded(['Schedule update [0]']);
-      expect(Scheduler).toFlushAndYieldThrough(['Count: 0']);
+      expect(Scheduler).toFlushAndYieldThrough([
+        'Schedule update [0]',
+        'Count: 0',
+      ]);
       expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);
 
       expect(Scheduler).toFlushAndYieldThrough(['Sync effect']);
@@ -862,7 +864,7 @@ describe('ReactHooksWithNoopRenderer', () => {
       expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
     });
 
-    it('flushes serial effects before enqueueing work', () => {
+    it('flushes passive effects when flushing discrete updates', () => {
       let _updateCount;
       function Counter(props) {
         const [count, updateCount] = useState(0);
@@ -880,15 +882,17 @@ describe('ReactHooksWithNoopRenderer', () => {
       expect(Scheduler).toFlushAndYieldThrough(['Count: 0', 'Sync effect']);
       expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
 
-      // Enqueuing this update forces the passive effect to be flushed --
+      // A discrete event forces the passive effect to be flushed --
       // updateCount(1) happens first, so 2 wins.
-      act(() => _updateCount(2));
+      ReactNoop.interactiveUpdates(() => {
+        act(() => _updateCount(2));
+      });
       expect(Scheduler).toHaveYielded(['Will set count to 1']);
       expect(Scheduler).toFlushAndYield(['Count: 2']);
       expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]);
     });
 
-    it('flushes serial effects before enqueueing work (with tracing)', () => {
+    it('flushes passive effects when flushing discrete updates (with tracing)', () => {
       const onInteractionScheduledWorkCompleted = jest.fn();
       const onWorkCanceled = jest.fn();
       SchedulerTracing.unstable_subscribe({
@@ -929,9 +933,11 @@ describe('ReactHooksWithNoopRenderer', () => {
 
       expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(0);
 
-      // Enqueuing this update forces the passive effect to be flushed --
+      // A discrete event forces the passive effect to be flushed --
       // updateCount(1) happens first, so 2 wins.
-      act(() => _updateCount(2));
+      ReactNoop.interactiveUpdates(() => {
+        act(() => _updateCount(2));
+      });
       expect(Scheduler).toHaveYielded(['Will set count to 1']);
       expect(Scheduler).toFlushAndYield(['Count: 2']);
       expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]);
@@ -1472,8 +1478,8 @@ describe('ReactHooksWithNoopRenderer', () => {
       ReactNoop.render(<Counter count={1} />, () =>
         Scheduler.yieldValue('Sync effect'),
       );
-      expect(Scheduler).toHaveYielded(['Mount normal [current: 0]']);
       expect(Scheduler).toFlushAndYieldThrough([
+        'Mount normal [current: 0]',
         'Unmount layout [current: 0]',
         'Mount layout [current: 1]',
         'Sync effect',