diff --git a/packages/react-dom/src/__tests__/ReactDOMHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMHooks-test.js index bc85d18b108ce..4f24770546176 100644 --- a/packages/react-dom/src/__tests__/ReactDOMHooks-test.js +++ b/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(, otherContainer); - }); - return null; - } - - ReactDOM.render(, container); - ReactDOM.unstable_batchedUpdates(() => { - _set(0); // Forces the effect to be flushed - expect(otherContainer.textContent).toBe('A'); - ReactDOM.render(, 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; diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 17fd298ef7fce..9c65a665c8eeb 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/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); }, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index b5b7f9dd79f7a..ffa4779eae712 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -31,7 +31,6 @@ import { import { scheduleWork, computeExpirationForFiber, - flushPassiveEffects, requestCurrentTime, warnIfNotCurrentlyActingUpdatesInDev, markRenderEventTime, @@ -1108,8 +1107,6 @@ function dispatchAction( lastRenderPhaseUpdate.next = update; } } else { - flushPassiveEffects(); - const currentTime = requestCurrentTime(); const expirationTime = computeExpirationForFiber(currentTime, fiber); diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index 3e1be7241e9d4..cae00c7b8964c 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/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, 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); }; diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 3de722bf11d21..09b5c2b4b0c71 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/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( // 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)); } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index e2e5178ba37f5..50276bd8b7e06 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/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 ? : null; + } + + function B(props) { + return ; + } + + 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(); + }); + + 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; diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 83cfd07dc91a1..b900086a49e74 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/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(, () => 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(, () => 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(, () => 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',