From 668fbd651b6b245da5c42e9e243adc88f0278517 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 14 May 2019 18:08:10 -0700 Subject: [PATCH] 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. --- .../src/__tests__/ReactDOMHooks-test.js | 36 ------------- .../src/ReactFiberClassComponent.js | 4 -- .../react-reconciler/src/ReactFiberHooks.js | 3 -- .../src/ReactFiberReconciler.js | 5 -- .../src/ReactFiberScheduler.js | 5 ++ .../src/__tests__/ReactHooks-test.internal.js | 52 +++++++++++++++++++ ...eactHooksWithNoopRenderer-test.internal.js | 32 +++++++----- 7 files changed, 76 insertions(+), 61 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMHooks-test.js index bc85d18b108c..4f2477054617 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 17fd298ef7fc..9c65a665c8ee 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 b5b7f9dd79f7..ffa4779eae71 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 3e1be7241e9d..cae00c7b8964 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 3de722bf11d2..09b5c2b4b0c7 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 e2e5178ba37f..50276bd8b7e0 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 83cfd07dc91a..b900086a49e7 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',