Skip to content

Commit

Permalink
[Fabric] Fix reparenting bug in legacy Suspense mount (#21995)
Browse files Browse the repository at this point in the history 
* Add reparenting invariant to React Noop

Fabric does not allow nodes to be reparented, so I added the equivalent
invariant to React Noop's so we can catch regressions.

This causes some tests to fail, which I'll fix in the next step.

* Fix: Use getOffscreenContainerProps

The type of these props is different per renderer. An oversight
from #21960. Unfortunately wasn't caught by Flow because fiber props
are `any`-typed.

* [Fabric] Fix reparenting in legacy Suspense mount

Fixes a weird case during legacy Suspense mount where the offscreen host
container of a tree that suspends during initial mount is recreated
instead of cloned, since there's no current fiber to clone from.

Fabric considers this a reparent even though the parent from the first
pass never committed.

Instead we can override the props from the first pass before the
container completes. It's a bit of a hack, but no more so than the rest
of the legacy root Suspense implementation — the hacks are designed
to make it usable by non-strict mode-compliant trees.
  • Loading branch information
@acdlite
acdlite authored Jul 30, 2021
1 parent 6f3fcbd commit f0d354e
Show file tree
Hide file tree
Showing 5 changed files with 96 additions and 38 deletions.
42 changes: 42 additions & 0 deletions packages/react-noop-renderer/src/createReactNoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type Props = {
type Instance = {|
type: string,
id: number,
parent: number,
children: Array<Instance | TextInstance>,
text: string | null,
prop: any,
Expand All @@ -57,6 +58,7 @@ type Instance = {|
type TextInstance = {|
text: string,
id: number,
parent: number,
hidden: boolean,
context: HostContext,
|};
Expand All @@ -80,6 +82,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
parentInstance: Container | Instance,
child: Instance | TextInstance,
): void {
const prevParent = child.parent;
if (prevParent !== -1 && prevParent !== parentInstance.id) {
throw new Error('Reparenting is not allowed');
}
child.parent = parentInstance.id;
const index = parentInstance.children.indexOf(child);
if (index !== -1) {
parentInstance.children.splice(index, 1);
Expand Down Expand Up @@ -211,6 +218,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
const clone = {
id: instance.id,
type: type,
parent: instance.parent,
children: keepChildren ? instance.children : [],
text: shouldSetTextContent(type, newProps)
? computeText((newProps.children: any) + '', instance.context)
Expand All @@ -223,6 +231,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
value: clone.id,
enumerable: false,
});
Object.defineProperty(clone, 'parent', {
value: clone.parent,
enumerable: false,
});
Object.defineProperty(clone, 'text', {
value: clone.text,
enumerable: false,
Expand Down Expand Up @@ -285,6 +297,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
id: instanceCounter++,
type: type,
children: [],
parent: -1,
text: shouldSetTextContent(type, props)
? computeText((props.children: any) + '', hostContext)
: null,
Expand All @@ -294,6 +307,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
};
// Hide from unit tests
Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false});
Object.defineProperty(inst, 'parent', {
value: inst.parent,
enumerable: false,
});
Object.defineProperty(inst, 'text', {
value: inst.text,
enumerable: false,
Expand All @@ -313,6 +330,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
parentInstance: Instance,
child: Instance | TextInstance,
): void {
const prevParent = child.parent;
if (prevParent !== -1 && prevParent !== parentInstance.id) {
throw new Error('Reparenting is not allowed');
}
child.parent = parentInstance.id;
parentInstance.children.push(child);
},

Expand Down Expand Up @@ -357,11 +379,16 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
const inst = {
text: text,
id: instanceCounter++,
parent: -1,
hidden: false,
context: hostContext,
};
// Hide from unit tests
Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false});
Object.defineProperty(inst, 'parent', {
value: inst.parent,
enumerable: false,
});
Object.defineProperty(inst, 'context', {
value: inst.context,
enumerable: false,
Expand Down Expand Up @@ -682,6 +709,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
const offscreenTextInstance: TextInstance = {
text: instance.text,
id: instanceCounter++,
parent: instance.parent,
hidden: hideNearestNode || instance.hidden,
context: instance.context,
};
Expand All @@ -690,6 +718,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
value: offscreenTextInstance.id,
enumerable: false,
});
Object.defineProperty(offscreenTextInstance, 'parent', {
value: offscreenTextInstance.parent,
enumerable: false,
});
Object.defineProperty(offscreenTextInstance, 'context', {
value: offscreenTextInstance.context,
enumerable: false,
Expand Down Expand Up @@ -725,6 +757,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
const clone: Instance = {
id: instance.id,
type: instance.type,
parent: instance.parent,
children: clonedChildren,
text: instance.text,
prop: instance.prop,
Expand All @@ -735,6 +768,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
value: clone.id,
enumerable: false,
});
Object.defineProperty(clone, 'parent', {
value: clone.parent,
enumerable: false,
});
Object.defineProperty(clone, 'text', {
value: clone.text,
enumerable: false,
Expand All @@ -754,13 +791,18 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
const clone = {
text: textInstance.text,
id: textInstance.id,
parent: textInstance.parent,
hidden: textInstance.hidden || hideNearestNode,
context: textInstance.context,
};
Object.defineProperty(clone, 'id', {
value: clone.id,
enumerable: false,
});
Object.defineProperty(clone, 'parent', {
value: clone.parent,
enumerable: false,
});
Object.defineProperty(clone, 'context', {
value: clone.context,
enumerable: false,
Expand Down
22 changes: 3 additions & 19 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2212,21 +2212,6 @@ function mountSuspenseFallbackChildren(
primaryChildFragment.childLanes = NoLanes;
primaryChildFragment.pendingProps = primaryChildProps;

if (
supportsPersistence &&
(workInProgress.mode & ConcurrentMode) === NoMode
) {
const isHidden = true;
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
const containerProps = {
hidden: isHidden,
primaryChildren,
};
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
completeSuspendedOffscreenHostContainer(null, offscreenContainer);
}

if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
// Reset the durations from the first pass so they aren't included in the
// final amounts. This seems counterintuitive, since we're intentionally
Expand Down Expand Up @@ -2373,13 +2358,12 @@ function updateSuspenseFallbackChildren(
// In persistent mode, the offscreen children are wrapped in a host node.
// We need to complete it now, because we're going to skip over its normal
// complete phase and go straight to rendering the fallback.
const isHidden = true;
const currentOffscreenContainer = currentPrimaryChildFragment.child;
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
const containerProps = {
hidden: isHidden,
const containerProps = getOffscreenContainerProps(
'hidden',
primaryChildren,
};
);
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
completeSuspendedOffscreenHostContainer(
Expand Down
22 changes: 3 additions & 19 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2212,21 +2212,6 @@ function mountSuspenseFallbackChildren(
primaryChildFragment.childLanes = NoLanes;
primaryChildFragment.pendingProps = primaryChildProps;

if (
supportsPersistence &&
(workInProgress.mode & ConcurrentMode) === NoMode
) {
const isHidden = true;
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
const containerProps = {
hidden: isHidden,
primaryChildren,
};
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
completeSuspendedOffscreenHostContainer(null, offscreenContainer);
}

if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
// Reset the durations from the first pass so they aren't included in the
// final amounts. This seems counterintuitive, since we're intentionally
Expand Down Expand Up @@ -2373,13 +2358,12 @@ function updateSuspenseFallbackChildren(
// In persistent mode, the offscreen children are wrapped in a host node.
// We need to complete it now, because we're going to skip over its normal
// complete phase and go straight to rendering the fallback.
const isHidden = true;
const currentOffscreenContainer = currentPrimaryChildFragment.child;
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
const containerProps = {
hidden: isHidden,
const containerProps = getOffscreenContainerProps(
'hidden',
primaryChildren,
};
);
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
completeSuspendedOffscreenHostContainer(
Expand Down
24 changes: 24 additions & 0 deletions packages/react-reconciler/src/ReactFiberThrow.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ import {
LifecycleEffectMask,
ForceUpdateForLegacySuspense,
} from './ReactFiberFlags';
import {
supportsPersistence,
getOffscreenContainerProps,
} from './ReactFiberHostConfig';
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.new';
import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode';
import {
Expand Down Expand Up @@ -313,6 +317,26 @@ function throwException(
// all lifecycle effect tags.
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);

if (supportsPersistence) {
// Another legacy Suspense quirk. In persistent mode, if this is the
// initial mount, override the props of the host container to hide
// its contents.
const currentSuspenseBoundary = workInProgress.alternate;
if (currentSuspenseBoundary === null) {
const offscreenFiber: Fiber = (workInProgress.child: any);
const offscreenContainer = offscreenFiber.child;
if (offscreenContainer !== null) {
const children = offscreenContainer.memoizedProps.children;
const containerProps = getOffscreenContainerProps(
'hidden',
children,
);
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
}
}
}

if (sourceFiber.tag === ClassComponent) {
const currentSourceFiber = sourceFiber.alternate;
if (currentSourceFiber === null) {
Expand Down
24 changes: 24 additions & 0 deletions packages/react-reconciler/src/ReactFiberThrow.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ import {
LifecycleEffectMask,
ForceUpdateForLegacySuspense,
} from './ReactFiberFlags';
import {
supportsPersistence,
getOffscreenContainerProps,
} from './ReactFiberHostConfig';
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.old';
import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode';
import {
Expand Down Expand Up @@ -313,6 +317,26 @@ function throwException(
// all lifecycle effect tags.
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);

if (supportsPersistence) {
// Another legacy Suspense quirk. In persistent mode, if this is the
// initial mount, override the props of the host container to hide
// its contents.
const currentSuspenseBoundary = workInProgress.alternate;
if (currentSuspenseBoundary === null) {
const offscreenFiber: Fiber = (workInProgress.child: any);
const offscreenContainer = offscreenFiber.child;
if (offscreenContainer !== null) {
const children = offscreenContainer.memoizedProps.children;
const containerProps = getOffscreenContainerProps(
'hidden',
children,
);
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
}
}
}

if (sourceFiber.tag === ClassComponent) {
const currentSourceFiber = sourceFiber.alternate;
if (currentSourceFiber === null) {
Expand Down

0 comments on commit f0d354e

Please sign in to comment.