[Fabric] Fix reparenting bug in legacy Suspense mount (facebook#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 facebook#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.
zhengjitf · Apr 15, 2022 · 3a77bd8 · 3a77bd8
1 parent 2a0f1bc
commit 3a77bd8
Show file tree

Hide file tree

Showing 5 changed files with 96 additions and 38 deletions.
diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js
@@ -48,6 +48,7 @@ type Props = {
 type Instance = {|
   type: string,
   id: number,
+  parent: number,
   children: Array<Instance | TextInstance>,
   text: string | null,
   prop: any,
@@ -57,6 +58,7 @@ type Instance = {|
 type TextInstance = {|
   text: string,
   id: number,
+  parent: number,
   hidden: boolean,
   context: HostContext,
 |};
@@ -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);
@@ -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)
@@ -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,
@@ -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,
@@ -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,
@@ -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);
     },
 
@@ -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,
@@ -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,
             };
@@ -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,
@@ -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,
@@ -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,
@@ -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,

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -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
@@ -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(

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js
@@ -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
@@ -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(

diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js
@@ -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 {
@@ -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) {

diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js
@@ -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 {
@@ -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) {