Fix serial passive effects

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',