From 78e816032c8af962343abbf384e06f3e9bae9269 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 21 Feb 2020 13:11:53 -0800 Subject: [PATCH] Don't warn about unmounted updates if pending passive unmount (#18096) I recently landed a change to the timing of passive effect cleanup functions during unmount (see #17925). This change defers flushing of passive effects for unmounted components until later (whenever we next flush pending passive effects). Since this change increases the likelihood of a (not actionable) state update warning for unmounted components, I've suppressed that warning for Fibers that have scheduled passive effect unmounts pending. --- .../src/ReactFiberWorkLoop.js | 13 ++ ...eactHooksWithNoopRenderer-test.internal.js | 178 ++++++++++++++++++ 2 files changed, 191 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 8c3e4dd170a8b..3c2427594a798 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -18,6 +18,7 @@ import type {Effect as HookEffect} from './ReactFiberHooks'; import { warnAboutDeprecatedLifecycles, + deferPassiveEffectCleanupDuringUnmount, runAllPassiveEffectDestroysBeforeCreates, enableUserTimingAPI, enableSuspenseServerRenderer, @@ -2687,6 +2688,18 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) { // Only warn for user-defined components, not internal ones like Suspense. return; } + + if ( + deferPassiveEffectCleanupDuringUnmount && + runAllPassiveEffectDestroysBeforeCreates + ) { + // If there are pending passive effects unmounts for this Fiber, + // we can assume that they would have prevented this update. + if (pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0) { + return; + } + } + // We show the whole stack but dedupe on the top component's name because // the problematic code almost always lies inside that component. const componentName = getComponentName(fiber.type) || 'ReactComponent'; diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 325d5ea7e7e00..81b774bd0a553 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -1144,6 +1144,184 @@ function loadModules({ ]); }); }); + + it('does not warn about state updates for unmounted components with pending passive unmounts', () => { + let completePendingRequest = null; + function Component() { + Scheduler.unstable_yieldValue('Component'); + const [didLoad, setDidLoad] = React.useState(false); + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('layout create'); + return () => { + Scheduler.unstable_yieldValue('layout destroy'); + }; + }, []); + React.useEffect(() => { + Scheduler.unstable_yieldValue('passive create'); + // Mimic an XHR request with a complete handler that updates state. + completePendingRequest = () => setDidLoad(true); + return () => { + Scheduler.unstable_yieldValue('passive destroy'); + }; + }, []); + return didLoad; + } + + act(() => { + ReactNoop.renderToRootWithID(, 'root', () => + Scheduler.unstable_yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYieldThrough([ + 'Component', + 'layout create', + 'Sync effect', + ]); + ReactNoop.flushPassiveEffects(); + expect(Scheduler).toHaveYielded(['passive create']); + + // Unmount but don't process pending passive destroy function + ReactNoop.unmountRootWithID('root'); + expect(Scheduler).toFlushAndYieldThrough(['layout destroy']); + + // Simulate an XHR completing, which will cause a state update- + // but should not log a warning. + completePendingRequest(); + + ReactNoop.flushPassiveEffects(); + expect(Scheduler).toHaveYielded(['passive destroy']); + }); + }); + + it('still warns about state updates for unmounted components with no pending passive unmounts', () => { + let completePendingRequest = null; + function Component() { + Scheduler.unstable_yieldValue('Component'); + const [didLoad, setDidLoad] = React.useState(false); + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('layout create'); + // Mimic an XHR request with a complete handler that updates state. + completePendingRequest = () => setDidLoad(true); + return () => { + Scheduler.unstable_yieldValue('layout destroy'); + }; + }, []); + return didLoad; + } + + act(() => { + ReactNoop.renderToRootWithID(, 'root', () => + Scheduler.unstable_yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYieldThrough([ + 'Component', + 'layout create', + 'Sync effect', + ]); + + // Unmount but don't process pending passive destroy function + ReactNoop.unmountRootWithID('root'); + expect(Scheduler).toFlushAndYieldThrough(['layout destroy']); + + // Simulate an XHR completing. + expect(completePendingRequest).toErrorDev( + "Warning: Can't perform a React state update on an unmounted component.", + ); + }); + }); + + it('still warns if there are pending passive unmount effects but not for the current fiber', () => { + let completePendingRequest = null; + function ComponentWithXHR() { + Scheduler.unstable_yieldValue('Component'); + const [didLoad, setDidLoad] = React.useState(false); + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('a:layout create'); + return () => { + Scheduler.unstable_yieldValue('a:layout destroy'); + }; + }, []); + React.useEffect(() => { + Scheduler.unstable_yieldValue('a:passive create'); + // Mimic an XHR request with a complete handler that updates state. + completePendingRequest = () => setDidLoad(true); + }, []); + return didLoad; + } + + function ComponentWithPendingPassiveUnmount() { + React.useEffect(() => { + Scheduler.unstable_yieldValue('b:passive create'); + return () => { + Scheduler.unstable_yieldValue('b:passive destroy'); + }; + }, []); + return null; + } + + act(() => { + ReactNoop.renderToRootWithID( + <> + + + , + 'root', + () => Scheduler.unstable_yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYieldThrough([ + 'Component', + 'a:layout create', + 'Sync effect', + ]); + ReactNoop.flushPassiveEffects(); + expect(Scheduler).toHaveYielded([ + 'a:passive create', + 'b:passive create', + ]); + + // Unmount but don't process pending passive destroy function + ReactNoop.unmountRootWithID('root'); + expect(Scheduler).toFlushAndYieldThrough(['a:layout destroy']); + + // Simulate an XHR completing in the component without a pending passive effect.. + expect(completePendingRequest).toErrorDev( + "Warning: Can't perform a React state update on an unmounted component.", + ); + }); + }); + + it('still warns about state updates from within passive unmount function', () => { + function Component() { + Scheduler.unstable_yieldValue('Component'); + const [didLoad, setDidLoad] = React.useState(false); + React.useEffect(() => { + Scheduler.unstable_yieldValue('passive create'); + return () => { + setDidLoad(true); + Scheduler.unstable_yieldValue('passive destroy'); + }; + }, []); + return didLoad; + } + + act(() => { + ReactNoop.renderToRootWithID(, 'root', () => + Scheduler.unstable_yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYieldThrough([ + 'Component', + 'Sync effect', + 'passive create', + ]); + + // Unmount but don't process pending passive destroy function + ReactNoop.unmountRootWithID('root'); + expect(() => { + expect(Scheduler).toFlushAndYield(['passive destroy']); + }).toErrorDev( + "Warning: Can't perform a React state update on an unmounted component.", + ); + }); + }); } it('updates have async priority', () => {