From ead0ccc8db0ff0450260307a028b685bbe88e133 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 8 May 2019 14:45:21 -0700 Subject: [PATCH 1/2] Failing test for false positive warning --- .../src/__tests__/ReactHooks-test.internal.js | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index e2e5178ba37f..3611ac08a113 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1719,6 +1719,57 @@ describe('ReactHooks', () => { ).toThrow('Hello'); }); + 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; From cb5609099d09fbc5aa5f9ad788ca8af88a7ba610 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 8 May 2019 14:43:46 -0700 Subject: [PATCH 2/2] 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 | 1 + ...eactHooksWithNoopRenderer-test.internal.js | 32 ++++++++++------- 7 files changed, 25 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 3611ac08a113..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,7 @@ 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; 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',