Skip to content

Effects list refactor #19261

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

Closed
wants to merge 8 commits into from
Closed
Changes from all 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
@@ -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;
8 changes: 8 additions & 0 deletions packages/react-reconciler/src/ReactChildFiber.new.js
Original file line number Diff line number Diff line change
@@ -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) {
returnFiber.deletions = [childToDelete];
} else {
returnFiber.deletions.push(childToDelete);
}
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
returnFiber.effectTag |= Deletion;
childToDelete.nextEffect = null;
childToDelete.effectTag = Deletion;
}
6 changes: 6 additions & 0 deletions packages/react-reconciler/src/ReactFiber.new.js
Original file line number Diff line number Diff line change
@@ -144,6 +144,8 @@ function FiberNode(

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

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

// The effect list is no longer valid.
workInProgress.nextEffect = null;
@@ -826,6 +830,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;
16 changes: 16 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
@@ -2066,6 +2066,13 @@ function updateSuspensePrimaryChildren(
currentFallbackChildFragment.nextEffect = null;
currentFallbackChildFragment.effectTag = Deletion;
workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
if (workInProgress.deletions === null) {
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;
@@ -2131,9 +2138,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(
@@ -3040,6 +3049,13 @@ function remountFiber(
} else {
returnFiber.firstEffect = returnFiber.lastEffect = current;
}
if (returnFiber.deletions === null) {
returnFiber.deletions = [current];
} else {
returnFiber.deletions.push(current);
}
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
returnFiber.effectTag |= Deletion;
current.nextEffect = null;
current.effectTag = Deletion;

6 changes: 6 additions & 0 deletions packages/react-reconciler/src/ReactFiberCompleteWork.new.js
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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 {
@@ -125,6 +125,13 @@ function deleteHydratableInstance(
childToDelete.stateNode = instance;
childToDelete.return = returnFiber;
childToDelete.effectTag = Deletion;
if (returnFiber.deletions === null) {
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.
544 changes: 314 additions & 230 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactInternalTypes.js
Original file line number Diff line number Diff line change
@@ -126,6 +126,8 @@ export type Fiber = {|

// Effect
effectTag: SideEffectTag,
subtreeTag: SideEffectTag,
deletions: Array<Fiber> | null,

// Singly linked list fast path to the next fiber with side-effects.
nextEffect: Fiber | null,
Original file line number Diff line number Diff line change
@@ -1743,7 +1743,6 @@ describe('ReactSuspenseWithNoopRenderer', () => {
await resolveText('B2');

expect(Scheduler).toHaveYielded(['Promise resolved [B2]']);

expect(Scheduler).toFlushAndYield([
'B2',
'Destroy Layout Effect [Loading...]',