Skip to content
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

Jump to bottom

Remove the warning for setState on unmounted components #22114

Merged
merged 2 commits into from
Aug 18, 2021
+12 −291
Merged

Remove the warning for setState on unmounted components #22114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ba8446
Remove warning for setState on unmounted components
gaearon Aug 17, 2021
72278da
Trigger CI
gaearon Aug 17, 2021
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
28 changes: 4 additions & 24 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ describe('ReactCompositeComponent', () => {
ReactDOM.render(<MyComponent />, container2);
});

it('should warn about `forceUpdate` on unmounted components', () => {
it('should not warn about `forceUpdate` on unmounted components', () => {
const container = document.createElement('div');
document.body.appendChild(container);

Expand All @@ -325,19 +325,11 @@ describe('ReactCompositeComponent', () => {

ReactDOM.unmountComponentAtNode(container);

expect(() => instance.forceUpdate()).toErrorDev(
"Warning: Can't perform a React state update on an unmounted " +
'component. This is a no-op, but it indicates a memory leak in your ' +
'application. To fix, cancel all subscriptions and asynchronous ' +
'tasks in the componentWillUnmount method.\n' +
' in Component (at **)',
);

// No additional warning should be recorded
instance.forceUpdate();
instance.forceUpdate();
});

it('should warn about `setState` on unmounted components', () => {
it('should not warn about `setState` on unmounted components', () => {
const container = document.createElement('div');
document.body.appendChild(container);

Expand Down Expand Up @@ -365,22 +357,10 @@ describe('ReactCompositeComponent', () => {
expect(renders).toBe(1);

instance.setState({value: 1});

expect(renders).toBe(2);

ReactDOM.render(<div />, container);

expect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

since we are not asserting anything here now, wouldn't this test pass even if the actual implementation DID throw a warning ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, our infra catches unexpected warnings.

instance.setState({value: 2});
}).toErrorDev(
"Warning: Can't perform a React state update on an unmounted " +
'component. This is a no-op, but it indicates a memory leak in your ' +
'application. To fix, cancel all subscriptions and asynchronous ' +
'tasks in the componentWillUnmount method.\n' +
' in Component (at **)\n' +
' in span',
);

instance.setState({value: 2});
expect(renders).toBe(2);
});

Expand Down
103 changes: 0 additions & 103 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import type {Fiber, FiberRoot} from './ReactInternalTypes';
import type {Lanes, Lane} from './ReactFiberLane.new';
import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
import type {StackCursor} from './ReactFiberStack.new';
import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new';
import type {Flags} from './ReactFiberFlags';

import {
Expand Down Expand Up @@ -53,10 +52,6 @@ import {
scheduleSyncCallback,
scheduleLegacySyncCallback,
} from './ReactFiberSyncTaskQueue.new';
import {
NoFlags as NoHookEffect,
Passive as HookPassive,
} from './ReactHookEffectTags';
import {
logCommitStarted,
logCommitStopped,
Expand Down Expand Up @@ -119,7 +114,6 @@ import {LegacyRoot} from './ReactRootTags';
import {
NoFlags,
Placement,
PassiveStatic,
Incomplete,
HostEffectMask,
Hydrating,
Expand Down Expand Up @@ -344,10 +338,6 @@ let nestedPassiveUpdateCount: number = 0;
let currentEventTime: number = NoTimestamp;
let currentEventTransitionLane: Lanes = NoLanes;

// Dev only flag that tracks if passive effects are currently being flushed.
// We warn about state updates for unmounted components differently in this case.
let isFlushingPassiveEffects = false;

export function getWorkInProgressRoot(): FiberRoot | null {
return workInProgressRoot;
}
Expand Down Expand Up @@ -454,7 +444,6 @@ export function scheduleUpdateOnFiber(

const root = markUpdateLaneFromFiberToRoot(fiber, lane);
if (root === null) {
warnAboutUpdateOnUnmountedFiberInDEV(fiber);
return null;
}

Expand Down Expand Up @@ -2048,10 +2037,6 @@ function flushPassiveEffectsImpl() {
markPassiveEffectsStarted(lanes);
}

if (__DEV__) {
isFlushingPassiveEffects = true;
}

const prevExecutionContext = executionContext;
executionContext |= CommitContext;

Expand All @@ -2068,10 +2053,6 @@ function flushPassiveEffectsImpl() {
}
}

if (__DEV__) {
isFlushingPassiveEffects = false;
}

if (__DEV__) {
if (enableDebugTracing) {
logPassiveEffectsStopped();
Expand Down Expand Up @@ -2503,90 +2484,6 @@ function warnAboutUpdateOnNotYetMountedFiberInDEV(fiber) {
}
}

let didWarnStateUpdateForUnmountedComponent: Set<string> | null = null;
function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
if (__DEV__) {
const tag = fiber.tag;
if (
tag !== HostRoot &&
tag !== ClassComponent &&
tag !== FunctionComponent &&
tag !== ForwardRef &&
tag !== MemoComponent &&
tag !== SimpleMemoComponent
) {
// Only warn for user-defined components, not internal ones like Suspense.
return;
}

if ((fiber.flags & PassiveStatic) !== NoFlags) {
const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
if (updateQueue !== null) {
const lastEffect = updateQueue.lastEffect;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

let effect = firstEffect;
do {
if (effect.destroy !== undefined) {
if ((effect.tag & HookPassive) !== NoHookEffect) {
return;
}
}
effect = effect.next;
} while (effect !== firstEffect);
}
}
}
// 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 = getComponentNameFromFiber(fiber) || 'ReactComponent';
if (didWarnStateUpdateForUnmountedComponent !== null) {
if (didWarnStateUpdateForUnmountedComponent.has(componentName)) {
return;
}
didWarnStateUpdateForUnmountedComponent.add(componentName);
} else {
didWarnStateUpdateForUnmountedComponent = new Set([componentName]);
}

if (isFlushingPassiveEffects) {
// Do not warn if we are currently flushing passive effects!
//
// React can't directly detect a memory leak, but there are some clues that warn about one.
// One of these clues is when an unmounted React component tries to update its state.
// For example, if a component forgets to remove an event listener when unmounting,
// that listener may be called later and try to update state,
// at which point React would warn about the potential leak.
//
// Warning signals are the most useful when they're strong.
// (So we should avoid false positive warnings.)
// Updating state from within an effect cleanup function is sometimes a necessary pattern, e.g.:
// 1. Updating an ancestor that a component had registered itself with on mount.
// 2. Resetting state when a component is hidden after going offscreen.
} else {
const previousFiber = ReactCurrentFiberCurrent;
try {
setCurrentDebugFiberInDEV(fiber);
console.error(
"Can't perform a React state update on an unmounted component. This " +
'is a no-op, but it indicates a memory leak in your application. To ' +
'fix, cancel all subscriptions and asynchronous tasks in %s.',
tag === ClassComponent
? 'the componentWillUnmount method'
: 'a useEffect cleanup function',
);
} finally {
if (previousFiber) {
setCurrentDebugFiberInDEV(fiber);
} else {
resetCurrentDebugFiberInDEV();
}
}
}
}
}

let beginWork;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
const dummyFiber = null;
Expand Down
103 changes: 0 additions & 103 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import type {Fiber, FiberRoot} from './ReactInternalTypes';
import type {Lanes, Lane} from './ReactFiberLane.old';
import type {SuspenseState} from './ReactFiberSuspenseComponent.old';
import type {StackCursor} from './ReactFiberStack.old';
import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.old';
import type {Flags} from './ReactFiberFlags';

import {
Expand Down Expand Up @@ -53,10 +52,6 @@ import {
scheduleSyncCallback,
scheduleLegacySyncCallback,
} from './ReactFiberSyncTaskQueue.old';
import {
NoFlags as NoHookEffect,
Passive as HookPassive,
} from './ReactHookEffectTags';
import {
logCommitStarted,
logCommitStopped,
Expand Down Expand Up @@ -119,7 +114,6 @@ import {LegacyRoot} from './ReactRootTags';
import {
NoFlags,
Placement,
PassiveStatic,
Incomplete,
HostEffectMask,
Hydrating,
Expand Down Expand Up @@ -344,10 +338,6 @@ let nestedPassiveUpdateCount: number = 0;
let currentEventTime: number = NoTimestamp;
let currentEventTransitionLane: Lanes = NoLanes;

// Dev only flag that tracks if passive effects are currently being flushed.
// We warn about state updates for unmounted components differently in this case.
let isFlushingPassiveEffects = false;

export function getWorkInProgressRoot(): FiberRoot | null {
return workInProgressRoot;
}
Expand Down Expand Up @@ -454,7 +444,6 @@ export function scheduleUpdateOnFiber(

const root = markUpdateLaneFromFiberToRoot(fiber, lane);
if (root === null) {
warnAboutUpdateOnUnmountedFiberInDEV(fiber);
return null;
}

Expand Down Expand Up @@ -2048,10 +2037,6 @@ function flushPassiveEffectsImpl() {
markPassiveEffectsStarted(lanes);
}

if (__DEV__) {
isFlushingPassiveEffects = true;
}

const prevExecutionContext = executionContext;
executionContext |= CommitContext;

Expand All @@ -2068,10 +2053,6 @@ function flushPassiveEffectsImpl() {
}
}

if (__DEV__) {
isFlushingPassiveEffects = false;
}

if (__DEV__) {
if (enableDebugTracing) {
logPassiveEffectsStopped();
Expand Down Expand Up @@ -2503,90 +2484,6 @@ function warnAboutUpdateOnNotYetMountedFiberInDEV(fiber) {
}
}

let didWarnStateUpdateForUnmountedComponent: Set<string> | null = null;
function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
if (__DEV__) {
const tag = fiber.tag;
if (
tag !== HostRoot &&
tag !== ClassComponent &&
tag !== FunctionComponent &&
tag !== ForwardRef &&
tag !== MemoComponent &&
tag !== SimpleMemoComponent
) {
// Only warn for user-defined components, not internal ones like Suspense.
return;
}

if ((fiber.flags & PassiveStatic) !== NoFlags) {
const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
if (updateQueue !== null) {
const lastEffect = updateQueue.lastEffect;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

let effect = firstEffect;
do {
if (effect.destroy !== undefined) {
if ((effect.tag & HookPassive) !== NoHookEffect) {
return;
}
}
effect = effect.next;
} while (effect !== firstEffect);
}
}
}
// 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 = getComponentNameFromFiber(fiber) || 'ReactComponent';
if (didWarnStateUpdateForUnmountedComponent !== null) {
if (didWarnStateUpdateForUnmountedComponent.has(componentName)) {
return;
}
didWarnStateUpdateForUnmountedComponent.add(componentName);
} else {
didWarnStateUpdateForUnmountedComponent = new Set([componentName]);
}

if (isFlushingPassiveEffects) {
// Do not warn if we are currently flushing passive effects!
//
// React can't directly detect a memory leak, but there are some clues that warn about one.
// One of these clues is when an unmounted React component tries to update its state.
// For example, if a component forgets to remove an event listener when unmounting,
// that listener may be called later and try to update state,
// at which point React would warn about the potential leak.
//
// Warning signals are the most useful when they're strong.
// (So we should avoid false positive warnings.)
// Updating state from within an effect cleanup function is sometimes a necessary pattern, e.g.:
// 1. Updating an ancestor that a component had registered itself with on mount.
// 2. Resetting state when a component is hidden after going offscreen.
} else {
const previousFiber = ReactCurrentFiberCurrent;
try {
setCurrentDebugFiberInDEV(fiber);
console.error(
"Can't perform a React state update on an unmounted component. This " +
'is a no-op, but it indicates a memory leak in your application. To ' +
'fix, cancel all subscriptions and asynchronous tasks in %s.',
tag === ClassComponent
? 'the componentWillUnmount method'
: 'a useEffect cleanup function',
);
} finally {
if (previousFiber) {
setCurrentDebugFiberInDEV(fiber);
} else {
resetCurrentDebugFiberInDEV();
}
}
}
}
}

let beginWork;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
const dummyFiber = null;
Expand Down
Loading