[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.
Andrew Clark committed
Jul 30, 2021 at 15:47 UTC
f0d354efc60633800aa30555ab5d890bcdfac0ae
5 files changed
+96
-38
packages/react-noop-renderer/src/createReactNoop.js
+42
@@ -48,6 +48,7 @@ type Props = {
48
type Instance = {|
49
type: string,
50
id: number,
51
+ parent: number,
52
children: Array<Instance | TextInstance>,
53
text: string | null,
54
prop: any,
@@ -57,6 +58,7 @@ type Instance = {|
58
type TextInstance = {|
59
text: string,
60
id: number,
61
+ parent: number,
62
hidden: boolean,
63
context: HostContext,
64
|};
@@ -80,6 +82,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
82
parentInstance: Container | Instance,
83
child: Instance | TextInstance,
84
): 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;
90
const index = parentInstance.children.indexOf(child);
91
if (index !== -1) {
92
parentInstance.children.splice(index, 1);
@@ -211,6 +218,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
218
const clone = {
219
id: instance.id,
220
type: type,
221
+ parent: instance.parent,
222
children: keepChildren ? instance.children : [],
223
text: shouldSetTextContent(type, newProps)
224
? computeText((newProps.children: any) + '', instance.context)
@@ -223,6 +231,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
231
value: clone.id,
232
enumerable: false,
233
});
234
+ Object.defineProperty(clone, 'parent', {
235
+ value: clone.parent,
236
+ enumerable: false,
237
+ });
238
Object.defineProperty(clone, 'text', {
239
value: clone.text,
240
enumerable: false,
@@ -285,6 +297,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
297
id: instanceCounter++,
298
type: type,
299
children: [],
300
+ parent: -1,
301
text: shouldSetTextContent(type, props)
302
? computeText((props.children: any) + '', hostContext)
303
: null,
@@ -294,6 +307,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
307
};
308
// Hide from unit tests
309
Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false});
310
+ Object.defineProperty(inst, 'parent', {
311
+ value: inst.parent,
312
+ enumerable: false,
313
+ });
314
Object.defineProperty(inst, 'text', {
315
value: inst.text,
316
enumerable: false,
@@ -313,6 +330,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
330
parentInstance: Instance,
331
child: Instance | TextInstance,
332
): 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;
338
parentInstance.children.push(child);
339
},
340
@@ -357,11 +379,16 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
379
const inst = {
380
text: text,
381
id: instanceCounter++,
382
+ parent: -1,
383
hidden: false,
384
context: hostContext,
385
};
386
// Hide from unit tests
387
Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false});
388
+ Object.defineProperty(inst, 'parent', {
389
+ value: inst.parent,
390
+ enumerable: false,
391
+ });
392
Object.defineProperty(inst, 'context', {
393
value: inst.context,
394
enumerable: false,
@@ -682,6 +709,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
709
const offscreenTextInstance: TextInstance = {
710
text: instance.text,
711
id: instanceCounter++,
712
+ parent: instance.parent,
713
hidden: hideNearestNode || instance.hidden,
714
context: instance.context,
715
};
@@ -690,6 +718,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
718
value: offscreenTextInstance.id,
719
enumerable: false,
720
});
721
+ Object.defineProperty(offscreenTextInstance, 'parent', {
722
+ value: offscreenTextInstance.parent,
723
+ enumerable: false,
724
+ });
725
Object.defineProperty(offscreenTextInstance, 'context', {
726
value: offscreenTextInstance.context,
727
enumerable: false,
@@ -725,6 +757,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
757
const clone: Instance = {
758
id: instance.id,
759
type: instance.type,
760
+ parent: instance.parent,
761
children: clonedChildren,
762
text: instance.text,
763
prop: instance.prop,
@@ -735,6 +768,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
768
value: clone.id,
769
enumerable: false,
770
});
771
+ Object.defineProperty(clone, 'parent', {
772
+ value: clone.parent,
773
+ enumerable: false,
774
+ });
775
Object.defineProperty(clone, 'text', {
776
value: clone.text,
777
enumerable: false,
@@ -754,6 +791,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
791
const clone = {
792
text: textInstance.text,
793
id: textInstance.id,
794
+ parent: textInstance.parent,
795
hidden: textInstance.hidden || hideNearestNode,
796
context: textInstance.context,
797
};
@@ -761,6 +799,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
799
value: clone.id,
800
enumerable: false,
801
});
802
+ Object.defineProperty(clone, 'parent', {
803
+ value: clone.parent,
804
+ enumerable: false,
805
+ });
806
Object.defineProperty(clone, 'context', {
807
value: clone.context,
808
enumerable: false,
packages/react-reconciler/src/ReactFiberBeginWork.new.js
+3
-19
@@ -2212,21 +2212,6 @@ function mountSuspenseFallbackChildren(
2212
primaryChildFragment.childLanes = NoLanes;
2213
primaryChildFragment.pendingProps = primaryChildProps;
2214
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
-
2215
if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
2216
// Reset the durations from the first pass so they aren't included in the
2217
// final amounts. This seems counterintuitive, since we're intentionally
@@ -2373,13 +2358,12 @@ function updateSuspenseFallbackChildren(
2358
// In persistent mode, the offscreen children are wrapped in a host node.
2359
// We need to complete it now, because we're going to skip over its normal
2360
// complete phase and go straight to rendering the fallback.
2376
- const isHidden = true;
2361
const currentOffscreenContainer = currentPrimaryChildFragment.child;
2362
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
2379
- const containerProps = {
2380
- hidden: isHidden,
2363
+ const containerProps = getOffscreenContainerProps(
2364
+ 'hidden',
2365
primaryChildren,
2382
- };
2366
+ );
2367
offscreenContainer.pendingProps = containerProps;
2368
offscreenContainer.memoizedProps = containerProps;
2369
completeSuspendedOffscreenHostContainer(
packages/react-reconciler/src/ReactFiberBeginWork.old.js
+3
-19
@@ -2212,21 +2212,6 @@ function mountSuspenseFallbackChildren(
2212
primaryChildFragment.childLanes = NoLanes;
2213
primaryChildFragment.pendingProps = primaryChildProps;
2214
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
-
2215
if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
2216
// Reset the durations from the first pass so they aren't included in the
2217
// final amounts. This seems counterintuitive, since we're intentionally
@@ -2373,13 +2358,12 @@ function updateSuspenseFallbackChildren(
2358
// In persistent mode, the offscreen children are wrapped in a host node.
2359
// We need to complete it now, because we're going to skip over its normal
2360
// complete phase and go straight to rendering the fallback.
2376
- const isHidden = true;
2361
const currentOffscreenContainer = currentPrimaryChildFragment.child;
2362
const offscreenContainer: Fiber = (primaryChildFragment.child: any);
2379
- const containerProps = {
2380
- hidden: isHidden,
2363
+ const containerProps = getOffscreenContainerProps(
2364
+ 'hidden',
2365
primaryChildren,
2382
- };
2366
+ );
2367
offscreenContainer.pendingProps = containerProps;
2368
offscreenContainer.memoizedProps = containerProps;
2369
completeSuspendedOffscreenHostContainer(
packages/react-reconciler/src/ReactFiberThrow.new.js
+24
@@ -33,6 +33,10 @@ import {
33
LifecycleEffectMask,
34
ForceUpdateForLegacySuspense,
35
} from './ReactFiberFlags';
36
+import {
37
+ supportsPersistence,
38
+ getOffscreenContainerProps,
39
+} from './ReactFiberHostConfig';
40
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.new';
41
import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode';
42
import {
@@ -313,6 +317,26 @@ function throwException(
317
// all lifecycle effect tags.
318
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
319
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
+
340
if (sourceFiber.tag === ClassComponent) {
341
const currentSourceFiber = sourceFiber.alternate;
342
if (currentSourceFiber === null) {
packages/react-reconciler/src/ReactFiberThrow.old.js
+24
@@ -33,6 +33,10 @@ import {
33
LifecycleEffectMask,
34
ForceUpdateForLegacySuspense,
35
} from './ReactFiberFlags';
36
+import {
37
+ supportsPersistence,
38
+ getOffscreenContainerProps,
39
+} from './ReactFiberHostConfig';
40
import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent.old';
41
import {NoMode, ConcurrentMode, DebugTracingMode} from './ReactTypeOfMode';
42
import {
@@ -313,6 +317,26 @@ function throwException(
317
// all lifecycle effect tags.
318
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
319
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
+
340
if (sourceFiber.tag === ClassComponent) {
341
const currentSourceFiber = sourceFiber.alternate;
342
if (currentSourceFiber === null) {