Skip to content

Commit f0d354e

Browse files
acdliteacdlite
authored
[Fabric] Fix reparenting bug in legacy Suspense mount (#21995)
* 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.
1 parent 6f3fcbd commit f0d354e

File tree

5 files changed

+96
-38
lines changed

5 files changed

+96
-38
lines changed

‎packages/react-noop-renderer/src/createReactNoop.js

+42
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ type Props = {
4848
type Instance = {|
4949
type: string,
5050
id: number,
51+
parent: number,
5152
children: Array<Instance | TextInstance>,
5253
text: string | null,
5354
prop: any,
@@ -57,6 +58,7 @@ type Instance = {|
5758
type TextInstance = {|
5859
text: string,
5960
id: number,
61+
parent: number,
6062
hidden: boolean,
6163
context: HostContext,
6264
|};
@@ -80,6 +82,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
8082
parentInstance: Container | Instance,
8183
child: Instance | TextInstance,
8284
): void {
85+
const prevParent = child.parent;
86+
if (prevParent !== -1 && prevParent !== parentInstance.id) {
87+
throw new Error('Reparenting is not allowed');
88+
}
89+
child.parent = parentInstance.id;
8390
const index = parentInstance.children.indexOf(child);
8491
if (index !== -1) {
8592
parentInstance.children.splice(index, 1);
@@ -211,6 +218,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
211218
const clone = {
212219
id: instance.id,
213220
type: type,
221+
parent: instance.parent,
214222
children: keepChildren ? instance.children : [],
215223
text: shouldSetTextContent(type, newProps)
216224
? computeText((newProps.children: any) + '', instance.context)
@@ -223,6 +231,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
223231
value: clone.id,
224232
enumerable: false,
225233
});
234+
Object.defineProperty(clone, 'parent', {
235+
value: clone.parent,
236+
enumerable: false,
237+
});
226238
Object.defineProperty(clone, 'text', {
227239
value: clone.text,
228240
enumerable: false,
@@ -285,6 +297,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
285297
id: instanceCounter++,
286298
type: type,
287299
children: [],
300+
parent: -1,
288301
text: shouldSetTextContent(type, props)
289302
? computeText((props.children: any) + '', hostContext)
290303
: null,
@@ -294,6 +307,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
294307
};
295308
// Hide from unit tests
296309
Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false});
310+
Object.defineProperty(inst, 'parent', {
311+
value: inst.parent,
312+
enumerable: false,
313+
});
297314
Object.defineProperty(inst, 'text', {
298315
value: inst.text,
299316
enumerable: false,
@@ -313,6 +330,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
313330
parentInstance: Instance,
314331
child: Instance | TextInstance,
315332
): void {
333+
const prevParent = child.parent;
334+
if (prevParent !== -1 && prevParent !== parentInstance.id) {
335+
throw new Error('Reparenting is not allowed');
336+
}
337+
child.parent = parentInstance.id;
316338
parentInstance.children.push(child);
317339
},
318340

@@ -357,11 +379,16 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
357379
const inst = {
358380
text: text,
359381
id: instanceCounter++,
382+
parent: -1,
360383
hidden: false,
361384
context: hostContext,
362385
};
363386
// Hide from unit tests
364387
Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false});
388+
Object.defineProperty(inst, 'parent', {
389+
value: inst.parent,
390+
enumerable: false,
391+
});
365392
Object.defineProperty(inst, 'context', {
366393
value: inst.context,
367394
enumerable: false,
@@ -682,6 +709,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
682709
const offscreenTextInstance: TextInstance = {
683710
text: instance.text,
684711
id: instanceCounter++,
712+
parent: instance.parent,
685713
hidden: hideNearestNode || instance.hidden,
686714
context: instance.context,
687715
};
@@ -690,6 +718,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
690718
value: offscreenTextInstance.id,
691719
enumerable: false,
692720
});
721+
Object.defineProperty(offscreenTextInstance, 'parent', {
722+
value: offscreenTextInstance.parent,
723+
enumerable: false,
724+
});
693725
Object.defineProperty(offscreenTextInstance, 'context', {
694726
value: offscreenTextInstance.context,
695727
enumerable: false,
@@ -725,6 +757,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
725757
const clone: Instance = {
726758
id: instance.id,
727759
type: instance.type,
760+
parent: instance.parent,
728761
children: clonedChildren,
729762
text: instance.text,
730763
prop: instance.prop,
@@ -735,6 +768,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
735768
value: clone.id,
736769
enumerable: false,
737770
});
771+
Object.defineProperty(clone, 'parent', {
772+
value: clone.parent,
773+
enumerable: false,
774+
});
738775
Object.defineProperty(clone, 'text', {
739776
value: clone.text,
740777
enumerable: false,
@@ -754,13 +791,18 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
754791
const clone = {
755792
text: textInstance.text,
756793
id: textInstance.id,
794+
parent: textInstance.parent,
757795
hidden: textInstance.hidden || hideNearestNode,
758796
context: textInstance.context,
759797
};
760798
Object.defineProperty(clone, 'id', {
761799
value: clone.id,
762800
enumerable: false,
763801
});
802+
Object.defineProperty(clone, 'parent', {
803+
value: clone.parent,
804+
enumerable: false,
805+
});
764806
Object.defineProperty(clone, 'context', {
765807
value: clone.context,
766808
enumerable: false,

‎packages/react-reconciler/src/ReactFiberBeginWork.new.js

+3-19
Original file line numberDiff line numberDiff line change
@@ -2212,21 +2212,6 @@ function mountSuspenseFallbackChildren(
22122212
primaryChildFragment.childLanes = NoLanes;
22132213
primaryChildFragment.pendingProps = primaryChildProps;
22142214

2215-
if (
2216-
supportsPersistence &&
2217-
(workInProgress.mode & ConcurrentMode) === NoMode
2218-
) {
2219-
const isHidden = true;
2220-
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
2221-
const containerProps = {
2222-
hidden: isHidden,
2223-
primaryChildren,
2224-
};
2225-
offscreenContainer.pendingProps = containerProps;
2226-
offscreenContainer.memoizedProps = containerProps;
2227-
completeSuspendedOffscreenHostContainer(null, offscreenContainer);
2228-
}
2229-
22302215
if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
22312216
// Reset the durations from the first pass so they aren't included in the
22322217
// final amounts. This seems counterintuitive, since we're intentionally
@@ -2373,13 +2358,12 @@ function updateSuspenseFallbackChildren(
23732358
// In persistent mode, the offscreen children are wrapped in a host node.
23742359
// We need to complete it now, because we're going to skip over its normal
23752360
// complete phase and go straight to rendering the fallback.
2376-
const isHidden = true;
23772361
const currentOffscreenContainer = currentPrimaryChildFragment.child;
23782362
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
2379-
const containerProps = {
2380-
hidden: isHidden,
2363+
const containerProps = getOffscreenContainerProps(
2364+
'hidden',
23812365
primaryChildren,
2382-
};
2366+
);
23832367
offscreenContainer.pendingProps = containerProps;
23842368
offscreenContainer.memoizedProps = containerProps;
23852369
completeSuspendedOffscreenHostContainer(

‎packages/react-reconciler/src/ReactFiberBeginWork.old.js

+3-19
Original file line numberDiff line numberDiff line change
@@ -2212,21 +2212,6 @@ function mountSuspenseFallbackChildren(
22122212
primaryChildFragment.childLanes = NoLanes;
22132213
primaryChildFragment.pendingProps = primaryChildProps;
22142214

2215-
if (
2216-
supportsPersistence &&
2217-
(workInProgress.mode & ConcurrentMode) === NoMode
2218-
) {
2219-
const isHidden = true;
2220-
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
2221-
const containerProps = {
2222-
hidden: isHidden,
2223-
primaryChildren,
2224-
};
2225-
offscreenContainer.pendingProps = containerProps;
2226-
offscreenContainer.memoizedProps = containerProps;
2227-
completeSuspendedOffscreenHostContainer(null, offscreenContainer);
2228-
}
2229-
22302215
if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
22312216
// Reset the durations from the first pass so they aren't included in the
22322217
// final amounts. This seems counterintuitive, since we're intentionally
@@ -2373,13 +2358,12 @@ function updateSuspenseFallbackChildren(
23732358
// In persistent mode, the offscreen children are wrapped in a host node.
23742359
// We need to complete it now, because we're going to skip over its normal
23752360
// complete phase and go straight to rendering the fallback.
2376-
const isHidden = true;
23772361
const currentOffscreenContainer = currentPrimaryChildFragment.child;
23782362
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
2379-
const containerProps = {
2380-
hidden: isHidden,
2363+
const containerProps = getOffscreenContainerProps(
2364+
'hidden',
23812365
primaryChildren,
2382-
};
2366+
);
23832367
offscreenContainer.pendingProps = containerProps;
23842368
offscreenContainer.memoizedProps = containerProps;
23852369
completeSuspendedOffscreenHostContainer(

‎packages/react-reconciler/src/ReactFiberThrow.new.js

+24
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ import {
3333
LifecycleEffectMask,
3434
ForceUpdateForLegacySuspense,
3535
} from './ReactFiberFlags';
36+
import {
37+
supportsPersistence,
38+
getOffscreenContainerProps,
39+
} from './ReactFiberHostConfig';
3640
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.new';
3741
import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode';
3842
import {
@@ -313,6 +317,26 @@ function throwException(
313317
// all lifecycle effect tags.
314318
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
315319

320+
if (supportsPersistence) {
321+
// Another legacy Suspense quirk. In persistent mode, if this is the
322+
// initial mount, override the props of the host container to hide
323+
// its contents.
324+
const currentSuspenseBoundary = workInProgress.alternate;
325+
if (currentSuspenseBoundary === null) {
326+
const offscreenFiber: Fiber = (workInProgress.child: any);
327+
const offscreenContainer = offscreenFiber.child;
328+
if (offscreenContainer !== null) {
329+
const children = offscreenContainer.memoizedProps.children;
330+
const containerProps = getOffscreenContainerProps(
331+
'hidden',
332+
children,
333+
);
334+
offscreenContainer.pendingProps = containerProps;
335+
offscreenContainer.memoizedProps = containerProps;
336+
}
337+
}
338+
}
339+
316340
if (sourceFiber.tag === ClassComponent) {
317341
const currentSourceFiber = sourceFiber.alternate;
318342
if (currentSourceFiber === null) {

‎packages/react-reconciler/src/ReactFiberThrow.old.js

+24
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ import {
3333
LifecycleEffectMask,
3434
ForceUpdateForLegacySuspense,
3535
} from './ReactFiberFlags';
36+
import {
37+
supportsPersistence,
38+
getOffscreenContainerProps,
39+
} from './ReactFiberHostConfig';
3640
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.old';
3741
import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode';
3842
import {
@@ -313,6 +317,26 @@ function throwException(
313317
// all lifecycle effect tags.
314318
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
315319

320+
if (supportsPersistence) {
321+
// Another legacy Suspense quirk. In persistent mode, if this is the
322+
// initial mount, override the props of the host container to hide
323+
// its contents.
324+
const currentSuspenseBoundary = workInProgress.alternate;
325+
if (currentSuspenseBoundary === null) {
326+
const offscreenFiber: Fiber = (workInProgress.child: any);
327+
const offscreenContainer = offscreenFiber.child;
328+
if (offscreenContainer !== null) {
329+
const children = offscreenContainer.memoizedProps.children;
330+
const containerProps = getOffscreenContainerProps(
331+
'hidden',
332+
children,
333+
);
334+
offscreenContainer.pendingProps = containerProps;
335+
offscreenContainer.memoizedProps = containerProps;
336+
}
337+
}
338+
}
339+
316340
if (sourceFiber.tag === ClassComponent) {
317341
const currentSourceFiber = sourceFiber.alternate;
318342
if (currentSourceFiber === null) {

0 commit comments

Comments
 (0)