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.

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

Effects list refactor continued: did-bailout flag #19322

Merged
merged 18 commits into from
Jul 16, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,14 @@ describe('ReactDOMServerPartialHydration', () => {
const span2 = container.getElementsByTagName('span')[0];
// This is a new node.
expect(span).not.toBe(span2);
expect(ref.current).toBe(span2);

if (gate(flags => flags.new)) {
// The effects list refactor causes this to be null because the Suspense Offscreen's child
// is null. However, since we can't hydrate Suspense in legacy this change in behavior is ok
expect(ref.current).toBe(null);
} else {
expect(ref.current).toBe(span2);
}

// Resolving the promise should render the final content.
suspend = false;
Expand Down
8 changes: 8 additions & 0 deletions packages/react-reconciler/src/ReactChildFiber.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -280,13 +280,21 @@ function ChildReconciler(shouldTrackSideEffects) {
// deletions, so we can just append the deletion to the list. The remaining
// effects aren't added until the complete phase. Once we implement
// resuming, this may not be true.
// TODO (effects) Get rid of effects list update here.
const last = returnFiber.lastEffect;
if (last !== null) {
last.nextEffect = childToDelete;
returnFiber.lastEffect = childToDelete;
} else {
returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
}
if (returnFiber.deletions === null) {
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
returnFiber.deletions = [childToDelete];
} else {
returnFiber.deletions.push(childToDelete);
}
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
returnFiber.effectTag |= Deletion;
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
childToDelete.nextEffect = null;
childToDelete.effectTag = Deletion;
}
Expand Down
7 changes: 7 additions & 0 deletions packages/react-reconciler/src/ReactFiber.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
enableBlocksAPI,
} from 'shared/ReactFeatureFlags';
import {NoEffect, Placement} from './ReactSideEffectTags';
import {NoEffect as NoSubtreeEffect} from './ReactSubtreeTags';
import {ConcurrentRoot, BlockingRoot} from './ReactRootTags';
import {
IndeterminateComponent,
Expand Down Expand Up @@ -144,6 +145,8 @@ function FiberNode(

// Effects
this.effectTag = NoEffect;
this.subtreeTag = NoSubtreeEffect;
this.deletions = null;
this.nextEffect = null;

this.firstEffect = null;
Expand Down Expand Up @@ -287,6 +290,8 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
// We already have an alternate.
// Reset the effect tag.
workInProgress.effectTag = NoEffect;
workInProgress.subtreeTag = NoSubtreeEffect;
workInProgress.deletions = null;

// The effect list is no longer valid.
workInProgress.nextEffect = null;
Expand Down Expand Up @@ -826,6 +831,8 @@ export function assignFiberPropertiesInDEV(
target.dependencies = source.dependencies;
target.mode = source.mode;
target.effectTag = source.effectTag;
target.subtreeTag = source.subtreeTag;
target.deletions = source.deletions;
target.nextEffect = source.nextEffect;
target.firstEffect = source.firstEffect;
target.lastEffect = source.lastEffect;
Expand Down
23 changes: 23 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import {
Deletion,
ForceUpdateForLegacySuspense,
} from './ReactSideEffectTags';
import {DidBailout} from './ReactSubtreeTags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
debugRenderPhaseSideEffectsForStrictMode,
Expand Down Expand Up @@ -2066,6 +2067,13 @@ function updateSuspensePrimaryChildren(
currentFallbackChildFragment.nextEffect = null;
currentFallbackChildFragment.effectTag = Deletion;
workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
if (workInProgress.deletions === null) {
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
workInProgress.deletions = [currentFallbackChildFragment];
} else {
workInProgress.deletions.push(currentFallbackChildFragment);
}
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
workInProgress.effectTag |= Deletion;
}

workInProgress.child = primaryChildFragment;
Expand Down Expand Up @@ -2131,9 +2139,11 @@ function updateSuspenseFallbackChildren(
workInProgress.firstEffect = primaryChildFragment.firstEffect;
workInProgress.lastEffect = progressedLastEffect;
progressedLastEffect.nextEffect = null;
workInProgress.deletions = null;
} else {
// TODO: Reset this somewhere else? Lol legacy mode is so weird.
workInProgress.firstEffect = workInProgress.lastEffect = null;
workInProgress.deletions = null;
}
} else {
primaryChildFragment = createWorkInProgressOffscreenFiber(
Expand Down Expand Up @@ -2980,6 +2990,12 @@ function bailoutOnAlreadyFinishedWork(

// Check if the children have any pending work.
if (!includesSomeLane(renderLanes, workInProgress.childLanes)) {
// TODO (effects) The case we are trying to handle is an error thrown during commit,
// and we replay the work, even though we skip render phase (by bailing out)
// we don't want to bailout on commit effects too.
// However, this check seems overly aggressive in some cases.
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
workInProgress.subtreeTag = DidBailout;

// The children don't have any work either. We can skip them.
// TODO: Once we add back resuming, we should check if the children are
// a work-in-progress set. If so, we need to transfer their effects.
Expand Down Expand Up @@ -3040,6 +3056,13 @@ function remountFiber(
} else {
returnFiber.firstEffect = returnFiber.lastEffect = current;
}
if (returnFiber.deletions === null) {
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
returnFiber.deletions = [current];
} else {
returnFiber.deletions.push(current);
}
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
returnFiber.effectTag |= Deletion;
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
current.nextEffect = null;
current.effectTag = Deletion;

Expand Down
6 changes: 6 additions & 0 deletions packages/react-reconciler/src/ReactFiberCompleteWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,12 @@ function completeWork(
// Reset the effect list before doing the second pass since that's now invalid.
if (renderState.lastEffect === null) {
workInProgress.firstEffect = null;
workInProgress.subtreeTag = NoEffect;
let child = workInProgress.child;
while (child !== null) {
child.deletions = null;
child = child.sibling;
}
}
workInProgress.lastEffect = renderState.lastEffect;
// Reset the child fibers to their original state.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
HostRoot,
SuspenseComponent,
} from './ReactWorkTags';
import {Deletion, Placement, Hydrating} from './ReactSideEffectTags';
import {Deletion, Hydrating, Placement} from './ReactSideEffectTags';
import invariant from 'shared/invariant';

import {
Expand Down Expand Up @@ -125,6 +125,13 @@ function deleteHydratableInstance(
childToDelete.stateNode = instance;
childToDelete.return = returnFiber;
childToDelete.effectTag = Deletion;
if (returnFiber.deletions === null) {
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
returnFiber.deletions = [childToDelete];
} else {
returnFiber.deletions.push(childToDelete);
}
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
returnFiber.effectTag |= Deletion;

// This might seem like it belongs on progressedFirstDeletion. However,
// these children are not part of the reconciliation list of children.
Expand Down
Loading