Skip to content

Don't warn about unmounted updates if pending passive unmount #18096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 21, 2020
Merged

Don't warn about unmounted updates if pending passive unmount #18096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
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;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odd, syntax highlighting seems to be broken for this file
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. Yeah for me me too.

// 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
178 changes: 178 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
View file
Open in desktop
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', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for when there's a pending unmount on a different component?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

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