Skip to content

Commit

Permalink
Don't warn about unmounted updates if pending passive unmount (#18096)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Brian Vaughn authored Feb 21, 2020
1 parent 2c4221c commit 78e8160
Show file tree
Hide file tree
Showing 2 changed files with 191 additions and 0 deletions.
13 changes: 13 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type {Effect as HookEffect} from './ReactFiberHooks';

import {
warnAboutDeprecatedLifecycles,
deferPassiveEffectCleanupDuringUnmount,
runAllPassiveEffectDestroysBeforeCreates,
enableUserTimingAPI,
enableSuspenseServerRenderer,
Expand Down Expand Up @@ -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';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(<Component />, '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(<Component />, '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(
<>
<ComponentWithXHR />
<ComponentWithPendingPassiveUnmount />
</>,
'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(<Component />, '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', () => {
Expand Down

0 comments on commit 78e8160

Please sign in to comment.