Skip to content

Commit

Permalink
Don't bailout after Suspending in Legacy Mode (#19216)
Browse files Browse the repository at this point in the history 
* Add a failing test for legacy Suspense blocking context updates in memo

* Add more test case coverage for variations of #17356

* Don't bailout after Suspending in Legacy Mode

Co-authored-by: Tharuka Devendra <[email protected]>
  • Loading branch information
@gaearon @Tsdevendra1
gaearon and Tsdevendra1 authored Jun 30, 2020
1 parent f4097c1 commit 8bff898
Show file tree
Hide file tree
Showing 6 changed files with 507 additions and 29 deletions.
24 changes: 19 additions & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import {
Update,
Ref,
Deletion,
ForceUpdateForLegacySuspense,
} from './ReactSideEffectTags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
Expand Down Expand Up @@ -548,6 +549,13 @@ function updateSimpleMemoComponent(
workInProgress,
renderLanes,
);
} else if (
(current.effectTag & ForceUpdateForLegacySuspense) !==
NoEffect
) {
// This is a special case that only exists for legacy mode.
// See https://github.com/facebook/react/pull/19216.
didReceiveUpdate = true;
}
}
}
Expand Down Expand Up @@ -3263,11 +3271,17 @@ function beginWork(
}
return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
} else {
// An update was scheduled on this fiber, but there are no new props
// nor legacy context. Set this to false. If an update queue or context
// consumer produces a changed value, it will set this to true. Otherwise,
// the component will assume the children have not changed and bail out.
didReceiveUpdate = false;
if ((current.effectTag & ForceUpdateForLegacySuspense) !== NoEffect) {
// This is a special case that only exists for legacy mode.
// See https://github.com/facebook/react/pull/19216.
didReceiveUpdate = true;
} else {
// An update was scheduled on this fiber, but there are no new props
// nor legacy context. Set this to false. If an update queue or context
// consumer produces a changed value, it will set this to true. Otherwise,
// the component will assume the children have not changed and bail out.
didReceiveUpdate = false;
}
}
} else {
didReceiveUpdate = false;
Expand Down
24 changes: 19 additions & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import {
Update,
Ref,
Deletion,
ForceUpdateForLegacySuspense,
} from './ReactSideEffectTags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
Expand Down Expand Up @@ -548,6 +549,13 @@ function updateSimpleMemoComponent(
workInProgress,
renderLanes,
);
} else if (
(current.effectTag & ForceUpdateForLegacySuspense) !==
NoEffect
) {
// This is a special case that only exists for legacy mode.
// See https://github.com/facebook/react/pull/19216.
didReceiveUpdate = true;
}
}
}
Expand Down Expand Up @@ -3263,11 +3271,17 @@ function beginWork(
}
return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
} else {
// An update was scheduled on this fiber, but there are no new props
// nor legacy context. Set this to false. If an update queue or context
// consumer produces a changed value, it will set this to true. Otherwise,
// the component will assume the children have not changed and bail out.
didReceiveUpdate = false;
if ((current.effectTag & ForceUpdateForLegacySuspense) !== NoEffect) {
// This is a special case that only exists for legacy mode.
// See https://github.com/facebook/react/pull/19216.
didReceiveUpdate = true;
} else {
// An update was scheduled on this fiber, but there are no new props
// nor legacy context. Set this to false. If an update queue or context
// consumer produces a changed value, it will set this to true. Otherwise,
// the component will assume the children have not changed and bail out.
didReceiveUpdate = false;
}
}
} else {
didReceiveUpdate = false;
Expand Down
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberThrow.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
NoEffect,
ShouldCapture,
LifecycleEffectMask,
ForceUpdateForLegacySuspense,
} from './ReactSideEffectTags';
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.new';
import {NoMode, BlockingMode} from './ReactTypeOfMode';
Expand Down Expand Up @@ -238,6 +239,7 @@ function throwException(
// should *not* suspend the commit.
if ((workInProgress.mode & BlockingMode) === NoMode) {
workInProgress.effectTag |= DidCapture;
sourceFiber.effectTag |= ForceUpdateForLegacySuspense;

// We're going to commit this fiber even though it didn't complete.
// But we shouldn't call any lifecycle methods or callbacks. Remove
Expand Down
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberThrow.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
NoEffect,
ShouldCapture,
LifecycleEffectMask,
ForceUpdateForLegacySuspense,
} from './ReactSideEffectTags';
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.old';
import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode';
Expand Down Expand Up @@ -249,6 +250,7 @@ function throwException(
// should *not* suspend the commit.
if ((workInProgress.mode & BlockingMode) === NoMode) {
workInProgress.effectTag |= DidCapture;
sourceFiber.effectTag |= ForceUpdateForLegacySuspense;

// We're going to commit this fiber even though it didn't complete.
// But we shouldn't call any lifecycle methods or callbacks. Remove
Expand Down
40 changes: 21 additions & 19 deletions packages/react-reconciler/src/ReactSideEffectTags.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,29 +10,31 @@
export type SideEffectTag = number;

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

// You can change the rest (and add more).
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 PassiveUnmountPendingDev = /* */ 0b10000000000000;
export const Hydrating = /* */ 0b00010000000000;
export const HydratingAndUpdate = /* */ 0b00010000000100;
export const Placement = /* */ 0b000000000000010;
export const Update = /* */ 0b000000000000100;
export const PlacementAndUpdate = /* */ 0b000000000000110;
export const Deletion = /* */ 0b000000000001000;
export const ContentReset = /* */ 0b000000000010000;
export const Callback = /* */ 0b000000000100000;
export const DidCapture = /* */ 0b000000001000000;
export const Ref = /* */ 0b000000010000000;
export const Snapshot = /* */ 0b000000100000000;
export const Passive = /* */ 0b000001000000000;
export const PassiveUnmountPendingDev = /* */ 0b010000000000000;
export const Hydrating = /* */ 0b000010000000000;
export const HydratingAndUpdate = /* */ 0b000010000000100;

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

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

export const Incomplete = /* */ 0b00100000000000;
export const ShouldCapture = /* */ 0b01000000000000;
// These are not really side effects, but we still reuse this field.
export const Incomplete = /* */ 0b000100000000000;
export const ShouldCapture = /* */ 0b001000000000000;
export const ForceUpdateForLegacySuspense = /* */ 0b100000000000000;
Loading

0 comments on commit 8bff898

Please sign in to comment.