Skip to content

Commit

Permalink
Fix incorrect unmounted state update warning
Browse files Browse the repository at this point in the history
We detach fibers (which nulls the  field) when we commit a deletion, so any state updates scheduled between that point and when we eventually flush passive effect destroys won't have a way to check if there is a pending passive unmount effect scheduled for its alternate unless we also explicitly track the alternates.
  • Loading branch information
Brian Vaughn committed Apr 15, 2020
1 parent b680174 commit aa44b1e
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 1 deletion.
16 changes: 15 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];
let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];
let pendingPassiveHookEffectsUnmountAlternatesDEV: Array<Fiber> = [];
let pendingPassiveProfilerEffects: Array<Fiber> = [];

let rootsWithPendingDiscreteUpdates: Map<
Expand Down Expand Up @@ -2208,6 +2209,13 @@ export function enqueuePendingPassiveHookEffectUnmount(
): void {
if (runAllPassiveEffectDestroysBeforeCreates) {
pendingPassiveHookEffectsUnmount.push(effect, fiber);
if (__DEV__) {
if (fiber.alternate !== null) {
// Alternate pointers get disconnected during unmount.
// Track alternates explicitly here to avoid warning about state updates for unmounted components.
pendingPassiveHookEffectsUnmountAlternatesDEV.push(fiber.alternate);
}
}
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
Expand Down Expand Up @@ -2257,6 +2265,9 @@ function flushPassiveEffectsImpl() {
// First pass: Destroy stale passive effects.
const unmountEffects = pendingPassiveHookEffectsUnmount;
pendingPassiveHookEffectsUnmount = [];
if (__DEV__) {
pendingPassiveHookEffectsUnmountAlternatesDEV = [];
}
for (let i = 0; i < unmountEffects.length; i += 2) {
const effect = ((unmountEffects[i]: any): HookEffect);
const fiber = ((unmountEffects[i + 1]: any): Fiber);
Expand Down Expand Up @@ -2735,7 +2746,10 @@ 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 (
pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0 ||
pendingPassiveHookEffectsUnmountAlternatesDEV.indexOf(fiber) >= 0
) {
return;
}
}
Expand Down
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

0 comments on commit aa44b1e

Please sign in to comment.