From 0a17ff198ee3c28af18cb4e90c6130799a9d3419 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 8 Mar 2019 15:55:57 +0000 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 087c8572cd66e..6a480958e5d83 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1699,6 +1699,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 dd4064aaf1e227fbcacc17cb805bb9ce50817b1f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 15 Mar 2019 23:44:32 +0000 Subject: [PATCH 2/2] Suppress unmounted setState warning after flushing passive effects --- .../src/ReactFiberScheduler.js | 37 ++++++++++++++----- .../src/__tests__/ReactHooks-test.internal.js | 17 +++++++++ 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 390f46af4ea4c..5cde36aa99e14 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -181,6 +181,7 @@ const {ReactCurrentDispatcher, ReactCurrentOwner} = ReactSharedInternals; let didWarnAboutStateTransition; let didWarnSetStateChildContext; +let suppressUpdateOnUnmountedWarning; let warnAboutUpdateOnUnmounted; let warnAboutInvalidUpdates; @@ -199,6 +200,7 @@ if (enableSchedulerTracing) { if (__DEV__) { didWarnAboutStateTransition = false; didWarnSetStateChildContext = false; + suppressUpdateOnUnmountedWarning = false; const didWarnStateUpdateForUnmountedComponent = {}; warnAboutUpdateOnUnmounted = function(fiber: Fiber, isClass: boolean) { @@ -605,6 +607,12 @@ function flushPassiveEffects() { // We call the scheduled callback instead of commitPassiveEffects directly // to ensure tracing works correctly. passiveEffectCallback(); + if (__DEV__) { + // Flushing passive effects could have led to unmounting a component + // that the user has already called setState on. So temporarily suppress + // any warnings about that until the next scheduleWork call. + suppressUpdateOnUnmountedWarning = true; + } } } @@ -1847,18 +1855,27 @@ export function warnIfNotCurrentlyBatchingInDev(fiber: Fiber): void { function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) { const root = scheduleWorkToRoot(fiber, expirationTime); + + let suppressWarning; + if (__DEV__) { + suppressWarning = suppressUpdateOnUnmountedWarning; + suppressUpdateOnUnmountedWarning = false; + } + if (root === null) { if (__DEV__) { - switch (fiber.tag) { - case ClassComponent: - warnAboutUpdateOnUnmounted(fiber, true); - break; - case FunctionComponent: - case ForwardRef: - case MemoComponent: - case SimpleMemoComponent: - warnAboutUpdateOnUnmounted(fiber, false); - break; + if (!suppressWarning) { + switch (fiber.tag) { + case ClassComponent: + warnAboutUpdateOnUnmounted(fiber, true); + break; + case FunctionComponent: + case ForwardRef: + case MemoComponent: + case SimpleMemoComponent: + warnAboutUpdateOnUnmounted(fiber, false); + break; + } } } return; diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 6a480958e5d83..e2b62b0922762 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1702,6 +1702,7 @@ describe('ReactHooks', () => { it('does not fire a false positive warning when previous effect unmounts the component', () => { let {useState, useEffect} = React; let globalListener; + let _setState; function A() { const [show, setShow] = useState(true); @@ -1721,10 +1722,17 @@ describe('ReactHooks', () => { useEffect(() => { let isStale = false; + _setState = setState; globalListener = () => { if (!isStale) { setState('hello'); } + if (!isStale) { + setState('goodbye'); + } + if (!isStale) { + setState('hello'); + } }; return () => { @@ -1748,6 +1756,15 @@ describe('ReactHooks', () => { // Note: should *not* warn about updates on unmounted component. // Because there's no way for component to know it got unmounted. ]); + + // Verify the warning isn't disabled permanently. + expect(() => { + ReactTestRenderer.act(() => { + _setState('bad'); + }); + }).toWarnDev([ + "Can't perform a React state update on an unmounted component", + ]); }); // Regression test for https://github.com/facebook/react/issues/14790