From 3a77bd881871c6952a217952e159a56a28048124 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 30 Jul 2021 15:47:40 -0400 Subject: [PATCH] [Fabric] Fix reparenting bug in legacy Suspense mount (#21995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- .../src/createReactNoop.js | 42 +++++++++++++++++++ .../src/ReactFiberBeginWork.new.js | 22 ++-------- .../src/ReactFiberBeginWork.old.js | 22 ++-------- .../src/ReactFiberThrow.new.js | 24 +++++++++++ .../src/ReactFiberThrow.old.js | 24 +++++++++++ 5 files changed, 96 insertions(+), 38 deletions(-) diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index f6af7341c2e0c..bc31a1c7c3cae 100644 --- 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, 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,6 +791,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { const clone = { text: textInstance.text, id: textInstance.id, + parent: textInstance.parent, hidden: textInstance.hidden || hideNearestNode, context: textInstance.context, }; @@ -761,6 +799,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { 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 index dd8ccc025cf4e..2321b97ff89e2 100644 --- 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 index bae769b1ce7d3..dc66340f70a53 100644 --- 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 index 874340a1eeaf2..e7816b017e473 100644 --- 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 index db11692c6ce39..70fda7bef55c1 100644 --- 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) {