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

Fix incorrect unmounted state update warning #18617

Merged
merged 3 commits into from
Apr 18, 2020
Merged

Fix incorrect unmounted state update warning #18617

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57164f2
Fix incorrect unmounted state update warning
Apr 15, 2020
5223a07
Use effectTag to suppress unmounted state update warnings
Apr 17, 2020
4537c13
Added "Dev" suffice to PendingPassiveUnmountDev flag
Apr 17, 2020
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
23 changes: 22 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ import {
Snapshot,
Callback,
Passive,
PassiveUnmountPending,
Incomplete,
HostEffectMask,
Hydrating,
Expand Down Expand Up @@ -2310,6 +2311,15 @@ export function enqueuePendingPassiveHookEffectUnmount(
): void {
if (runAllPassiveEffectDestroysBeforeCreates) {
pendingPassiveHookEffectsUnmount.push(effect, fiber);
if (__DEV__) {
if (deferPassiveEffectCleanupDuringUnmount) {
fiber.effectTag |= PassiveUnmountPending;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it weird to only set this bit in DEV mode? It's only needed there.

Copy link
Collaborator

@gaearon gaearon Apr 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Add Dev suffix to the flag?

const alternate = fiber.alternate;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Can you pass current as an argument through schedulePassiveEffects? We try to limit the number of places we access the .alternate pointer, since one day we're hoping to remove i t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do that for schedulePassiveEffects() because commitLifeCycles() already has both fibers.

But we also call enqueuePendingPassiveHookEffectUnmount() from commitUnmount() which is called from commitNestedUnmounts() which traces all the through to commitMutationEffects(), so I'm still going to have to access alternate there (and add a bunch of additional params to the functions in between).

Is this still the right change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like what we're calling "current" is inconsistent between commitWork and commitDeletion calls.

For e.g. commitWork we pass nextEffect.alternate as current:

react/packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines 2192 to 2193 in cfefc81

const current = nextEffect.alternate;
commitWork(current, nextEffect);

But for commitDeletion we pass nextEffect as current:

react/packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Line 2214 in cfefc81

commitDeletion(root, nextEffect, renderPriorityLevel);

Copy link
Contributor Author

@bvaughn bvaughn Apr 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We also call commitNestedUnmounts and commitUnmount from unmountHostComponents while traversing the tree, so we'd need to add a bunch of additional alternate access here to pass through the added param.

Since this was a "nit" and given the above considerations, I think I'm not going to make any additional change to the alternate stuff for now. Seems like it would be a net loss. We can always follow up with another PR if someone disagrees strongly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right I guess “current” isn’t accurate, it’s just the “other fiber”

Yeah that makes sense. If we got rid of alternates, each fiber would have a shared instance object and we could put the flag on that.

if (alternate !== null) {
alternate.effectTag |= PassiveUnmountPending;
}
}
}
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
Expand Down Expand Up @@ -2372,6 +2382,17 @@ function flushPassiveEffectsImpl() {
const fiber = ((unmountEffects[i + 1]: any): Fiber);
const destroy = effect.destroy;
effect.destroy = undefined;

if (__DEV__) {
if (deferPassiveEffectCleanupDuringUnmount) {
fiber.effectTag &= ~PassiveUnmountPending;
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.effectTag &= ~PassiveUnmountPending;
}
}
}

if (typeof destroy === 'function') {
if (__DEV__) {
setCurrentDebugFiberInDEV(fiber);
Expand Down Expand Up @@ -2851,7 +2872,7 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
) {
// 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) {
if ((fiber.effectTag & PassiveUnmountPending) !== NoEffect) {
return;
}
}
Expand Down
23 changes: 22 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ import {
Snapshot,
Callback,
Passive,
PassiveUnmountPending,
Incomplete,
HostEffectMask,
Hydrating,
Expand Down Expand Up @@ -2329,6 +2330,15 @@ export function enqueuePendingPassiveHookEffectUnmount(
): void {
if (runAllPassiveEffectDestroysBeforeCreates) {
pendingPassiveHookEffectsUnmount.push(effect, fiber);
if (__DEV__) {
if (deferPassiveEffectCleanupDuringUnmount) {
fiber.effectTag |= PassiveUnmountPending;
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.effectTag |= PassiveUnmountPending;
}
}
}
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
Expand Down Expand Up @@ -2391,6 +2401,17 @@ function flushPassiveEffectsImpl() {
const fiber = ((unmountEffects[i + 1]: any): Fiber);
const destroy = effect.destroy;
effect.destroy = undefined;

if (__DEV__) {
if (deferPassiveEffectCleanupDuringUnmount) {
fiber.effectTag &= ~PassiveUnmountPending;
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.effectTag &= ~PassiveUnmountPending;
}
}
}

if (typeof destroy === 'function') {
if (__DEV__) {
setCurrentDebugFiberInDEV(fiber);
Expand Down Expand Up @@ -2873,7 +2894,7 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
) {
// 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) {
if ((fiber.effectTag & PassiveUnmountPending) !== NoEffect) {
return;
}
}
Expand Down
37 changes: 19 additions & 18 deletions packages/react-reconciler/src/ReactSideEffectTags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,29 @@
export type SideEffectTag = number;

// Don't change these two values. They're used by React Dev Tools.
export const NoEffect = /* */ 0b0000000000000;
export const PerformedWork = /* */ 0b0000000000001;
export const NoEffect = /* */ 0b00000000000000;
export const PerformedWork = /* */ 0b00000000000001;

// You can change the rest (and add more).
export const Placement = /* */ 0b0000000000010;
export const Update = /* */ 0b0000000000100;
export const PlacementAndUpdate = /* */ 0b0000000000110;
export const Deletion = /* */ 0b0000000001000;
export const ContentReset = /* */ 0b0000000010000;
export const Callback = /* */ 0b0000000100000;
export const DidCapture = /* */ 0b0000001000000;
export const Ref = /* */ 0b0000010000000;
export const Snapshot = /* */ 0b0000100000000;
export const Passive = /* */ 0b0001000000000;
export const Hydrating = /* */ 0b0010000000000;
export const HydratingAndUpdate = /* */ 0b0010000000100;
export const Placement = /* */ 0b00000000000010;
export const Update = /* */ 0b00000000000100;
export const PlacementAndUpdate = /* */ 0b00000000000110;
export const Deletion = /* */ 0b00000000001000;
export const ContentReset = /* */ 0b00000000010000;
export const Callback = /* */ 0b00000000100000;
export const DidCapture = /* */ 0b00000001000000;
export const Ref = /* */ 0b00000010000000;
export const Snapshot = /* */ 0b00000100000000;
export const Passive = /* */ 0b00001000000000;
export const PassiveUnmountPending = /* */ 0b10000000000000;
export const Hydrating = /* */ 0b00010000000000;
export const HydratingAndUpdate = /* */ 0b00010000000100;

// Passive & Update & Callback & Ref & Snapshot
export const LifecycleEffectMask = /* */ 0b0001110100100;
export const LifecycleEffectMask = /* */ 0b00001110100100;

// Union of all host effects
export const HostEffectMask = /* */ 0b0011111111111;
export const HostEffectMask = /* */ 0b00011111111111;

export const Incomplete = /* */ 0b0100000000000;
export const ShouldCapture = /* */ 0b1000000000000;
export const Incomplete = /* */ 0b00100000000000;
export const ShouldCapture = /* */ 0b01000000000000;
89 changes: 89 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,95 @@ describe('ReactHooksWithNoopRenderer', () => {
});
});

// @gate deferPassiveEffectCleanupDuringUnmount && runAllPassiveEffectDestroysBeforeCreates
it('does not warn about state updates for unmounted components with pending passive unmounts for alternates', () => {
let setParentState = null;
const setChildStates = [];

function Parent() {
const [state, setState] = useState(true);
setParentState = setState;
Scheduler.unstable_yieldValue(`Parent ${state} render`);
useLayoutEffect(() => {
Scheduler.unstable_yieldValue(`Parent ${state} commit`);
});
if (state) {
return (
<>
<Child label="one" />
<Child label="two" />
</>
);
} else {
return null;
}
}

function Child({label}) {
const [state, setState] = useState(0);
useLayoutEffect(() => {
Scheduler.unstable_yieldValue(`Child ${label} commit`);
});
useEffect(() => {
setChildStates.push(setState);
Scheduler.unstable_yieldValue(`Child ${label} passive create`);
return () => {
Scheduler.unstable_yieldValue(`Child ${label} passive destroy`);
};
}, []);
Scheduler.unstable_yieldValue(`Child ${label} render`);
return state;
}

// Schedule debounced state update for child (prob a no-op for this test)
// later tick: schedule unmount for parent
// start process unmount (but don't flush passive effectS)
// State update on child
act(() => {
ReactNoop.render(<Parent />);
expect(Scheduler).toFlushAndYieldThrough([
'Parent true render',
'Child one render',
'Child two render',
'Child one commit',
'Child two commit',
'Parent true commit',
'Child one passive create',
'Child two passive create',
]);

// Update children.
setChildStates.forEach(setChildState => setChildState(1));
expect(Scheduler).toFlushAndYieldThrough([
'Child one render',
'Child two render',
'Child one commit',
'Child two commit',
]);

// Schedule another update for children, and partially process it.
setChildStates.forEach(setChildState => setChildState(2));
expect(Scheduler).toFlushAndYieldThrough(['Child one render']);

// Schedule unmount for the parent that unmounts children with pending update.
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => setParentState(false),
);
expect(Scheduler).toFlushAndYieldThrough([
'Parent false render',
'Parent false commit',
]);

// Schedule updates for children too (which should be ignored)
setChildStates.forEach(setChildState => setChildState(2));
expect(Scheduler).toFlushAndYield([
'Child one passive destroy',
'Child two passive destroy',
]);
});
});

it('warns about state updates for unmounted components with no pending passive unmounts', () => {
let completePendingRequest = null;
function Component() {
Expand Down