Skip to content

Commit

Permalink
Reset new fork to old fork (#20254)
Browse files Browse the repository at this point in the history 
* Fix typo

This typo was fixed in the new fork but not the old.

* Reset new fork to old fork

Something in the new fork is causing a topline metrics regression. We're
not sure what it is, so we're going to split it into steps and bisect.

As a first step, this resets the new fork back to the contents of the
old fork. We will land this to confirm that the fork infra itself is
not causing a regression.

* Fix tests: Add `dfsEffectsRefactor` flag

Some of the tests that gated on the effects refactor used the `new`
flag. In order to bisect, we'll need to decompose the new fork changes
into multiple steps.

So I added a hardcoded test flag called `dfsEffectsRefactor` and set it
to false. Will turn back on when we switch back to traversing the
finished tree using DFS and `subtreeTag`.
  • Loading branch information
@acdlite
acdlite authored Nov 13, 2020
1 parent 7548dd5 commit 765e89b
Show file tree
Hide file tree
Showing 19 changed files with 1,380 additions and 2,552 deletions.
2 changes: 1 addition & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ describe('ReactDOMServerPartialHydration', () => {
// This is a new node.
expect(span).not.toBe(span2);

if (gate(flags => flags.new)) {
if (gate(flags => flags.dfsEffectsRefactor)) {
// 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);
Expand Down
1 change: 1 addition & 0 deletions packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ beforeEach(() => {
});

// Don't feel too guilty if you have to delete this test.
// @gate dfsEffectsRefactor
// @gate new
// @gate __DEV__
test('warns in DEV if return pointer is inconsistent', async () => {
Expand Down
19 changes: 13 additions & 6 deletions packages/react-reconciler/src/ReactChildFiber.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import type {Fiber} from './ReactInternalTypes';
import type {Lanes} from './ReactFiberLane';

import getComponentName from 'shared/getComponentName';
import {Deletion, Placement} from './ReactFiberFlags';
import {Placement, Deletion} from './ReactFiberFlags';
import {
getIteratorFn,
REACT_ELEMENT_TYPE,
Expand Down Expand Up @@ -256,13 +256,20 @@ function ChildReconciler(shouldTrackSideEffects) {
// Noop.
return;
}
const deletions = returnFiber.deletions;
if (deletions === null) {
returnFiber.deletions = [childToDelete];
returnFiber.flags |= Deletion;
// Deletions are added in reversed order so we add it to the front.
// At this point, the return fiber's effect list is empty except for
// 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.
const last = returnFiber.lastEffect;
if (last !== null) {
last.nextEffect = childToDelete;
returnFiber.lastEffect = childToDelete;
} else {
deletions.push(childToDelete);
returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
}
childToDelete.nextEffect = null;
childToDelete.flags = Deletion;
}

function deleteRemainingChildren(
Expand Down
17 changes: 13 additions & 4 deletions packages/react-reconciler/src/ReactFiber.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
enableFundamentalAPI,
enableScopeAPI,
} from 'shared/ReactFeatureFlags';
import {NoFlags, Placement, StaticMask} from './ReactFiberFlags';
import {NoFlags, Placement} from './ReactFiberFlags';
import {ConcurrentRoot, BlockingRoot} from './ReactRootTags';
import {
IndeterminateComponent,
Expand Down Expand Up @@ -279,6 +279,13 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
workInProgress.type = current.type;

// We already have an alternate.
// Reset the effect tag.
workInProgress.flags = NoFlags;

// The effect list is no longer valid.
workInProgress.nextEffect = null;
workInProgress.firstEffect = null;
workInProgress.lastEffect = null;
workInProgress.subtreeFlags = NoFlags;
workInProgress.deletions = null;

Expand All @@ -292,9 +299,6 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
}
}

// Reset all effects except static ones.
// Static effects are not specific to a render.
workInProgress.flags = current.flags & StaticMask;
workInProgress.childLanes = current.childLanes;
workInProgress.lanes = current.lanes;

Expand Down Expand Up @@ -360,6 +364,11 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
// that child fiber is setting, not the reconciliation.
workInProgress.flags &= Placement;

// The effect list is no longer valid.
workInProgress.nextEffect = null;
workInProgress.firstEffect = null;
workInProgress.lastEffect = null;

const current = workInProgress.alternate;
if (current === null) {
// Reset to createFiber's initial values.
Expand Down
68 changes: 42 additions & 26 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ import {
Hydrating,
ContentReset,
DidCapture,
Update,
Ref,
Deletion,
ForceUpdateForLegacySuspense,
StaticMask,
} from './ReactFiberFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
Expand Down Expand Up @@ -671,6 +671,8 @@ function updateProfiler(
renderLanes: Lanes,
) {
if (enableProfilerTimer) {
workInProgress.flags |= Update;

// Reset effect durations for the next eventual effect phase.
// These are reset during render to allow the DevTools commit hook a chance to read them,
const stateNode = workInProgress.stateNode;
Expand Down Expand Up @@ -1077,9 +1079,6 @@ function updateHostComponent(
workInProgress.flags |= ContentReset;
}

// React DevTools reads this flag.
workInProgress.flags |= PerformedWork;

markRef(current, workInProgress);
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
return workInProgress.child;
Expand Down Expand Up @@ -2005,14 +2004,9 @@ function updateSuspensePrimaryChildren(
primaryChildFragment.sibling = null;
if (currentFallbackChildFragment !== null) {
// Delete the fallback child fragment
const deletions = workInProgress.deletions;
if (deletions === null) {
workInProgress.deletions = [currentFallbackChildFragment];
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
workInProgress.flags |= Deletion;
} else {
deletions.push(currentFallbackChildFragment);
}
currentFallbackChildFragment.nextEffect = null;
currentFallbackChildFragment.flags = Deletion;
workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
}

workInProgress.child = primaryChildFragment;
Expand Down Expand Up @@ -2069,19 +2063,24 @@ function updateSuspenseFallbackChildren(

// The fallback fiber was added as a deletion effect during the first pass.
// However, since we're going to remain on the fallback, we no longer want
// to delete it.
workInProgress.deletions = null;
// to delete it. So we need to remove it from the list. Deletions are stored
// on the same list as effects. We want to keep the effects from the primary
// tree. So we copy the primary child fragment's effect list, which does not
// include the fallback deletion effect.
const progressedLastEffect = primaryChildFragment.lastEffect;
if (progressedLastEffect !== null) {
workInProgress.firstEffect = primaryChildFragment.firstEffect;
workInProgress.lastEffect = progressedLastEffect;
progressedLastEffect.nextEffect = null;
} else {
// TODO: Reset this somewhere else? Lol legacy mode is so weird.
workInProgress.firstEffect = workInProgress.lastEffect = null;
}
} else {
primaryChildFragment = createWorkInProgressOffscreenFiber(
currentPrimaryChildFragment,
primaryChildProps,
);

// Since we're reusing a current tree, we need to reuse the flags, too.
// (We don't do this in legacy mode, because in legacy mode we don't re-use
// the current tree; see previous branch.)
primaryChildFragment.subtreeFlags =
currentPrimaryChildFragment.subtreeFlags & StaticMask;
}
let fallbackChildFragment;
if (currentFallbackChildFragment !== null) {
Expand Down Expand Up @@ -2566,6 +2565,7 @@ function initSuspenseListRenderState(
tail: null | Fiber,
lastContentRow: null | Fiber,
tailMode: SuspenseListTailMode,
lastEffectBeforeRendering: null | Fiber,
): void {
const renderState: null | SuspenseListRenderState =
workInProgress.memoizedState;
Expand All @@ -2577,6 +2577,7 @@ function initSuspenseListRenderState(
last: lastContentRow,
tail: tail,
tailMode: tailMode,
lastEffect: lastEffectBeforeRendering,
}: SuspenseListRenderState);
} else {
// We can reuse the existing object from previous renders.
Expand All @@ -2586,6 +2587,7 @@ function initSuspenseListRenderState(
renderState.last = lastContentRow;
renderState.tail = tail;
renderState.tailMode = tailMode;
renderState.lastEffect = lastEffectBeforeRendering;
}
}

Expand Down Expand Up @@ -2667,6 +2669,7 @@ function updateSuspenseListComponent(
tail,
lastContentRow,
tailMode,
workInProgress.lastEffect,
);
break;
}
Expand Down Expand Up @@ -2698,6 +2701,7 @@ function updateSuspenseListComponent(
tail,
null, // last
tailMode,
workInProgress.lastEffect,
);
break;
}
Expand All @@ -2708,6 +2712,7 @@ function updateSuspenseListComponent(
null, // tail
null, // last
undefined,
workInProgress.lastEffect,
);
break;
}
Expand Down Expand Up @@ -2967,14 +2972,15 @@ function remountFiber(

// Delete the old fiber and place the new one.
// Since the old fiber is disconnected, we have to schedule it manually.
const deletions = returnFiber.deletions;
if (deletions === null) {
returnFiber.deletions = [current];
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
returnFiber.flags |= Deletion;
const last = returnFiber.lastEffect;
if (last !== null) {
last.nextEffect = current;
returnFiber.lastEffect = current;
} else {
deletions.push(current);
returnFiber.firstEffect = returnFiber.lastEffect = current;
}
current.nextEffect = null;
current.flags = Deletion;

newWorkInProgress.flags |= Placement;

Expand Down Expand Up @@ -3059,6 +3065,15 @@ function beginWork(
}
case Profiler:
if (enableProfilerTimer) {
// Profiler should only call onRender when one of its descendants actually rendered.
const hasChildWork = includesSomeLane(
renderLanes,
workInProgress.childLanes,
);
if (hasChildWork) {
workInProgress.flags |= Update;
}

// Reset effect durations for the next eventual effect phase.
// These are reset during render to allow the DevTools commit hook a chance to read them,
const stateNode = workInProgress.stateNode;
Expand Down Expand Up @@ -3165,6 +3180,7 @@ function beginWork(
// update in the past but didn't complete it.
renderState.rendering = null;
renderState.tail = null;
renderState.lastEffect = null;
}
pushSuspenseContext(workInProgress, suspenseStackCursor.current);

Expand Down
52 changes: 6 additions & 46 deletions packages/react-reconciler/src/ReactFiberClassComponent.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,13 @@ import type {Lanes} from './ReactFiberLane';
import type {UpdateQueue} from './ReactUpdateQueue.new';

import * as React from 'react';
import {Update, Snapshot, MountLayoutDev} from './ReactFiberFlags';
import {Update, Snapshot} from './ReactFiberFlags';
import {
debugRenderPhaseSideEffectsForStrictMode,
disableLegacyContext,
enableDebugTracing,
enableSchedulingProfiler,
warnAboutDeprecatedLifecycles,
enableDoubleInvokingEffects,
} from 'shared/ReactFeatureFlags';
import ReactStrictModeWarnings from './ReactStrictModeWarnings.new';
import {isMounted} from './ReactFiberTreeReflection';
Expand All @@ -30,13 +29,7 @@ import invariant from 'shared/invariant';
import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols';

import {resolveDefaultProps} from './ReactFiberLazyComponent.new';
import {
BlockingMode,
ConcurrentMode,
DebugTracingMode,
NoMode,
StrictMode,
} from './ReactTypeOfMode';
import {DebugTracingMode, StrictMode} from './ReactTypeOfMode';

import {
enqueueUpdate,
Expand Down Expand Up @@ -897,16 +890,7 @@ function mountClassInstance(
}

if (typeof instance.componentDidMount === 'function') {
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
) {
// Never double-invoke effects for legacy roots.
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
workInProgress.flags |= Update;
}
}

Expand Down Expand Up @@ -976,15 +960,7 @@ function resumeMountClassInstance(
// If an update was already in progress, we should schedule an Update
// effect even though we're bailing out, so that cWU/cDU are called.
if (typeof instance.componentDidMount === 'function') {
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
workInProgress.flags |= Update;
}
return false;
}
Expand Down Expand Up @@ -1027,29 +1003,13 @@ function resumeMountClassInstance(
}
}
if (typeof instance.componentDidMount === 'function') {
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
workInProgress.flags |= Update;
}
} else {
// If an update was already in progress, we should schedule an Update
// effect even though we're bailing out, so that cWU/cDU are called.
if (typeof instance.componentDidMount === 'function') {
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
workInProgress.flags |= Update;
}

// If shouldComponentUpdate returned false, we should still update the
Expand Down
Loading

0 comments on commit 765e89b

Please sign in to comment.