From 9bcd5b25761f63755ca98575ee0107e1defbeb97 Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 5 Apr 2019 00:29:47 +0100 Subject: [PATCH 1/6] Fix Suspense fragment edge cases --- shells/dev/app/SuspenseTree/index.js | 64 ++++++++++++++++++++++++++++ shells/dev/app/index.js | 2 + src/backend/renderer.js | 30 ++++++++++++- 3 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 shells/dev/app/SuspenseTree/index.js diff --git a/shells/dev/app/SuspenseTree/index.js b/shells/dev/app/SuspenseTree/index.js new file mode 100644 index 0000000000..fb4ca80de6 --- /dev/null +++ b/shells/dev/app/SuspenseTree/index.js @@ -0,0 +1,64 @@ +// @flow + +import React, { Suspense, useState } from 'react'; + +function SuspenseTree() { + return ( + <> +

Suspense

+ Loading outer}> + + + + ); +} + +function Parent() { + return ( +
+ Loading inner 1}> + Hello + + Loading inner 2}> + World + + This will never load}> + + + +
+ ); +} + +function LoadLater() { + const [loadChild, setLoadChild] = useState(0); + return ( + setLoadChild(true)}>Click to load + } + > + {loadChild ? ( + setLoadChild(false)}> + Loaded! Click to suspend again. + + ) : ( + + )} + + ); +} + +function Child(props) { + return

; +} + +function Fallback(props) { + return

{props.children}

; +} + +function Never() { + throw new Promise(resolve => {}); +} + +export default SuspenseTree; diff --git a/shells/dev/app/index.js b/shells/dev/app/index.js index 4857c0c2cf..3a76db783f 100644 --- a/shells/dev/app/index.js +++ b/shells/dev/app/index.js @@ -11,6 +11,7 @@ import InspectableElements from './InspectableElements'; import InteractionTracing from './InteractionTracing'; import ToDoList from './ToDoList'; import Toggle from './Toggle'; +import SuspenseTree from './SuspenseTree'; import './styles.css'; @@ -33,6 +34,7 @@ function mountTestApp() { mountHelper(ElementTypes); mountHelper(EditableProps); mountHelper(Toggle); + mountHelper(SuspenseTree); mountHelper(DeeplyNestedComponents); } diff --git a/src/backend/renderer.js b/src/backend/renderer.js index cf0b973e68..099e0b3743 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -716,6 +716,16 @@ export function attach( function enqueueUnmount(fiber) { const isRoot = fiber.tag === HostRoot; const primaryFiber = getPrimaryFiber(fiber); + if (!fiberToIDMap.has(primaryFiber)) { + // If we've never seen this Fiber, it might be because + // it is inside a non-current Suspense fragment tree, + // and so the store is not even aware of it. + // In that case we can just ignore it, or otherwise + // there will be errors later on. + primaryFibers.delete(primaryFiber); + // TODO: this is fragile and can obscure actual bugs. + return; + } const id = getFiberID(primaryFiber); if (isRoot) { const operation = new Uint32Array(2); @@ -757,8 +767,24 @@ export function attach( enqueueMount(fiber, parentFiber); } - if (fiber.child !== null) { - mountFiber(fiber.child, shouldEnqueueMount ? fiber : parentFiber, true); + const isTimedOutSuspense = + fiber.tag === ReactTypeOfWork.SuspenseComponent && + fiber.memoizedState !== null; + + if (isTimedOutSuspense) { + // Special case: if Suspense mounts in a timed-out state, + // get the fallback child from the inner fragment and mount + // it as if it was our own child. Updates handle this too. + const primaryChildFragment = fiber.child; + const fallbackChildFragment = primaryChildFragment.sibling; + const fallbackChild = fallbackChildFragment.child; + if (fallbackChild !== null) { + mountFiber(fallbackChild, shouldEnqueueMount ? fiber : parentFiber, true); + } + } else { + if (fiber.child !== null) { + mountFiber(fiber.child, shouldEnqueueMount ? fiber : parentFiber, true); + } } if (traverseSiblings && fiber.sibling !== null) { From d6257d382d6f9910fb36ad1b4707bfa23ac965d2 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 5 Apr 2019 14:57:40 +0100 Subject: [PATCH 2/6] Expand the test fixture --- shells/dev/app/SuspenseTree/index.js | 109 ++++++++++++++++++++++----- 1 file changed, 90 insertions(+), 19 deletions(-) diff --git a/shells/dev/app/SuspenseTree/index.js b/shells/dev/app/SuspenseTree/index.js index fb4ca80de6..1e7fec726d 100644 --- a/shells/dev/app/SuspenseTree/index.js +++ b/shells/dev/app/SuspenseTree/index.js @@ -6,7 +6,66 @@ function SuspenseTree() { return ( <>

Suspense

- Loading outer}> + + + + ); +} + +function PrimaryFallbackTest() { + const [suspend, setSuspend] = useState(false); + const fallbackStep = useTestSequence('fallback', Fallback1, Fallback2); + const primaryStep = useTestSequence('primary', Primary1, Primary2); + return ( + <> +

Suspense Primary / Fallback

+ +
+ + {suspend ? : primaryStep} + + + ); +} + +function useTestSequence(label, T1, T2) { + let [step, setStep] = useState(0); + let next = ( + + ); + let allSteps = [ + <>{next}, + <> + {next} mount + , + <> + {next} update + , + <> + {next} several different{' '} + children + , + <> + {next} goodbye + , + ]; + return allSteps[step]; +} + +function NestedSuspenseTest() { + return ( + <> +

Nested Suspense

+ Loading outer}> @@ -16,16 +75,20 @@ function SuspenseTree() { function Parent() { return (
- Loading inner 1}> - Hello + Loading inner 1}> + Hello + {' '} + Loading inner 2}> + World - Loading inner 2}> - World - - This will never load}> +
+ This will never load}> - +
+ + +
); } @@ -35,13 +98,13 @@ function LoadLater() { return ( setLoadChild(true)}>Click to load + setLoadChild(true)}>Click to load } > {loadChild ? ( - setLoadChild(false)}> + setLoadChild(false)}> Loaded! Click to suspend again. - + ) : ( )} @@ -49,16 +112,24 @@ function LoadLater() { ); } -function Child(props) { - return

; -} - -function Fallback(props) { - return

{props.children}

; -} - function Never() { throw new Promise(resolve => {}); } +function Fallback1({ prop, ...rest }) { + return ; +} + +function Fallback2({ prop, ...rest }) { + return ; +} + +function Primary1({ prop, ...rest }) { + return ; +} + +function Primary2({ prop, ...rest }) { + return ; +} + export default SuspenseTree; From 0f536bba5c4354838d4dc1f3fdf226d1b0e67ee9 Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 5 Apr 2019 21:48:50 +0100 Subject: [PATCH 3/6] Rewrite the Suspense logic --- src/backend/renderer.js | 253 +++++++++++++++++++++++++--------------- src/constants.js | 1 + src/devtools/store.js | 55 ++++++++- 3 files changed, 209 insertions(+), 100 deletions(-) diff --git a/src/backend/renderer.js b/src/backend/renderer.js index 099e0b3743..db87d89cb2 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -22,6 +22,7 @@ import { TREE_OPERATION_ADD, TREE_OPERATION_REMOVE, TREE_OPERATION_RESET_CHILDREN, + TREE_OPERATION_RECURSIVE_REMOVE_CHILDREN, TREE_OPERATION_UPDATE_TREE_BASE_DURATION, } from '../constants'; import { getUID } from '../utils'; @@ -627,7 +628,7 @@ export function attach( pendingOperations = new Uint32Array(0); } - function enqueueMount(fiber: Fiber, parentFiber: Fiber | null) { + function recordMount(fiber: Fiber, parentFiber: Fiber | null) { const isRoot = fiber.tag === HostRoot; const id = getFiberID(getPrimaryFiber(fiber)); @@ -713,7 +714,7 @@ export function attach( } } - function enqueueUnmount(fiber) { + function recordUnmount(fiber) { const isRoot = fiber.tag === HostRoot; const primaryFiber = getPrimaryFiber(fiber); if (!fiberToIDMap.has(primaryFiber)) { @@ -753,18 +754,27 @@ export function attach( } } - function mountFiber( + function recordRecursiveRemoveChildren(fiber) { + const primaryFiber = getPrimaryFiber(fiber); + const id = getFiberID(primaryFiber); + const operation = new Uint32Array(2); + operation[0] = TREE_OPERATION_RECURSIVE_REMOVE_CHILDREN; + operation[1] = id; + addOperation(operation, false); + } + + function mountFiberRecursively( fiber: Fiber, parentFiber: Fiber | null, traverseSiblings = false ) { if (__DEBUG__) { - debug('mountFiber()', fiber, parentFiber); + debug('mountFiberRecursively()', fiber, parentFiber); } - const shouldEnqueueMount = !shouldFilterFiber(fiber); - if (shouldEnqueueMount) { - enqueueMount(fiber, parentFiber); + const shouldInclude = !shouldFilterFiber(fiber); + if (shouldInclude) { + recordMount(fiber, parentFiber); } const isTimedOutSuspense = @@ -779,25 +789,45 @@ export function attach( const fallbackChildFragment = primaryChildFragment.sibling; const fallbackChild = fallbackChildFragment.child; if (fallbackChild !== null) { - mountFiber(fallbackChild, shouldEnqueueMount ? fiber : parentFiber, true); + mountFiberRecursively( + fallbackChild, + shouldInclude ? fiber : parentFiber, + true + ); } } else { if (fiber.child !== null) { - mountFiber(fiber.child, shouldEnqueueMount ? fiber : parentFiber, true); + mountFiberRecursively( + fiber.child, + shouldInclude ? fiber : parentFiber, + true + ); } } if (traverseSiblings && fiber.sibling !== null) { - mountFiber(fiber.sibling, parentFiber, true); + mountFiberRecursively(fiber.sibling, parentFiber, true); } } - function enqueueUpdateIfNecessary( - fiber: Fiber, - hasChildOrderChanged: boolean - ) { + function unmountFiberRecursively(fiber, traverseSiblings = false) { if (__DEBUG__) { - debug('enqueueUpdateIfNecessary()', fiber); + debug('unmountFiberRecursively()', fiber, traverseSiblings); + } + if (!shouldFilterFiber(fiber)) { + recordUnmount(fiber); + } + if (fiber.child !== null) { + unmountFiberRecursively(fiber.child, true); + } + if (traverseSiblings && fiber.sibling !== null) { + unmountFiberRecursively(fiber.sibling, true); + } + } + + function maybeRecordUpdate(fiber: Fiber, hasChildOrderChanged: boolean) { + if (__DEBUG__) { + debug('maybeRecordUpdate()', fiber); } const isProfilingSupported = fiber.hasOwnProperty('treeBaseDuration'); @@ -844,7 +874,7 @@ export function attach( // We might want to revisit this if it proves to be too inefficient. let child = fiber.child; while (child !== null) { - findReorderedChildren(child, nextChildren); + findReorderedChildrenRecursively(child, nextChildren); child = child.sibling; } @@ -858,109 +888,140 @@ export function attach( } } - function findReorderedChildren(fiber: Fiber, nextChildren: Array) { + function findReorderedChildrenRecursively( + fiber: Fiber, + nextChildren: Array + ) { if (!shouldFilterFiber(fiber)) { nextChildren.push(getFiberID(getPrimaryFiber(fiber))); } else { let child = fiber.child; while (child !== null) { - findReorderedChildren(child, nextChildren); + findReorderedChildrenRecursively(child, nextChildren); child = child.sibling; } } } - function updateFiber( + function updateFiberRecursively( nextFiber: Fiber, prevFiber: Fiber, parentFiber: Fiber | null ) { if (__DEBUG__) { - debug('enqueueUpdateIfNecessary()', nextFiber, parentFiber); + debug('updateFiberRecursively()', nextFiber, parentFiber); } - const shouldEnqueueUpdate = !shouldFilterFiber(nextFiber); + // The behavior of timed-out Suspense trees is unique. + // Rather than unmount the timed out content (and possibly lose important state), + // React re-parents this content within a hidden Fragment while the fallback is showing. + // This behavior doesn't need to be observable in the DevTools though. + // It might even result in a bad user experience for e.g. node selection in the Elements panel. + // The easiest fix is to strip out the intermediate Fragment fibers, + // so the Elements panel and Profiler don't need to special case them. + if (nextFiber.tag === SuspenseComponent) { + // Suspense components only have a non-null memoizedState if they're timed-out. + const prevDidTimeout = prevFiber.memoizedState !== null; + const nextDidTimeOut = nextFiber.memoizedState !== null; - // Suspense components only have a non-null memoizedState if they're timed-out. - const isTimedOutSuspense = - nextFiber.tag === SuspenseComponent && nextFiber.memoizedState !== null; - - if (isTimedOutSuspense) { - // The behavior of timed-out Suspense trees is unique. - // Rather than unmount the timed out content (and possibly lose important state), - // React re-parents this content within a hidden Fragment while the fallback is showing. - // This behavior doesn't need to be observable in the DevTools though. - // It might even result in a bad user experience for e.g. node selection in the Elements panel. - // The easiest fix is to strip out the intermediate Fragment fibers, - // so the Elements panel and Profiler don't need to special case them. - const primaryChildFragment = nextFiber.child; - const fallbackChildFragment = primaryChildFragment.sibling; - const fallbackChild = fallbackChildFragment.child; - - // The primary, hidden child is never actually updated in this case, - // so we can skip any updates to its tree. - // We only need to track updates to the Fallback UI for now. - if (fallbackChild.alternate) { - updateFiber(fallbackChild, fallbackChild.alternate, nextFiber); + // The logic below is inspired by the codepaths in updateSuspenseComponent() + // inside ReactFiberBeginWork in the React source code. + if (prevDidTimeout) { + if (nextDidTimeOut) { + // Fallback -> Fallback: + // 1. Reconcile fallback set. + const nextFallbackChildSet = nextFiber.child.sibling; + // Note: We can't use nextFiber.child.sibling.alternate + // because the set is special and alternate may not exist. + const prevFallbackChildSet = prevFiber.child.sibling; + updateFiberRecursively( + nextFallbackChildSet, + prevFallbackChildSet, + nextFiber + ); + return; + } else { + // Fallback -> Primary: + // 1. Unmount fallback set + // Note: don't emulate fallback unmount because React actually did it. + // 2. Mount primary set + const nextPrimaryChildSet = nextFiber.child; + mountFiberRecursively(nextPrimaryChildSet, nextFiber, true); + return; + } } else { - mountFiber(fallbackChild, nextFiber); + if (nextDidTimeOut) { + // Primary -> Fallback: + // 1. Hide primary set + // This is not a real unmount, so it won't get reported by React. + // By this point it's *too late* to find the previous primary child set + // so we'll just tell the store to "forget" about those children. + // They might "resurface" later when we switch to primary content, + // but from the store's point of view they will be a new tree. + recordRecursiveRemoveChildren(nextFiber); + // 2. Mount fallback set + const nextFallbackChildSet = nextFiber.child.sibling; + mountFiberRecursively(nextFallbackChildSet, nextFiber, true); + return; + } else { + // Primary -> Primary: + // 1. Reconcile primary set. + // Note: no return so we can passthrough to the logic below. + } } + } - if (shouldEnqueueUpdate) { - enqueueUpdateIfNecessary(nextFiber, false); - } - } else { - let hasChildOrderChanged = false; - if (nextFiber.child !== prevFiber.child) { - // If the first child is different, we need to traverse them. - // Each next child will be either a new child (mount) or an alternate (update). - let nextChild = nextFiber.child; - let prevChildAtSameIndex = prevFiber.child; - while (nextChild) { - // We already know children will be referentially different because - // they are either new mounts or alternates of previous children. - // Schedule updates and mounts depending on whether alternates exist. - // We don't track deletions here because they are reported separately. - if (nextChild.alternate) { - const prevChild = nextChild.alternate; - updateFiber( - nextChild, - prevChild, - shouldEnqueueUpdate ? nextFiber : parentFiber - ); - // However we also keep track if the order of the children matches - // the previous order. They are always different referentially, but - // if the instances line up conceptually we'll want to know that. - if (!hasChildOrderChanged && prevChild !== prevChildAtSameIndex) { - hasChildOrderChanged = true; - } - } else { - mountFiber( - nextChild, - shouldEnqueueUpdate ? nextFiber : parentFiber - ); - if (!hasChildOrderChanged) { - hasChildOrderChanged = true; - } + const shouldInclude = !shouldFilterFiber(nextFiber); + let hasChildOrderChanged = false; + if (nextFiber.child !== prevFiber.child) { + // If the first child is different, we need to traverse them. + // Each next child will be either a new child (mount) or an alternate (update). + let nextChild = nextFiber.child; + let prevChildAtSameIndex = prevFiber.child; + while (nextChild) { + // We already know children will be referentially different because + // they are either new mounts or alternates of previous children. + // Schedule updates and mounts depending on whether alternates exist. + // We don't track deletions here because they are reported separately. + if (nextChild.alternate) { + const prevChild = nextChild.alternate; + updateFiberRecursively( + nextChild, + prevChild, + shouldInclude ? nextFiber : parentFiber + ); + // However we also keep track if the order of the children matches + // the previous order. They are always different referentially, but + // if the instances line up conceptually we'll want to know that. + if (!hasChildOrderChanged && prevChild !== prevChildAtSameIndex) { + hasChildOrderChanged = true; } - // Try the next child. - nextChild = nextChild.sibling; - // Advance the pointer in the previous list so that we can - // keep comparing if they line up. - if (!hasChildOrderChanged && prevChildAtSameIndex != null) { - prevChildAtSameIndex = prevChildAtSameIndex.sibling; + } else { + mountFiberRecursively( + nextChild, + shouldInclude ? nextFiber : parentFiber + ); + if (!hasChildOrderChanged) { + hasChildOrderChanged = true; } } - // If we have no more children, but used to, they don't line up. + // Try the next child. + nextChild = nextChild.sibling; + // Advance the pointer in the previous list so that we can + // keep comparing if they line up. if (!hasChildOrderChanged && prevChildAtSameIndex != null) { - hasChildOrderChanged = true; + prevChildAtSameIndex = prevChildAtSameIndex.sibling; } } - - if (shouldEnqueueUpdate) { - enqueueUpdateIfNecessary(nextFiber, hasChildOrderChanged); + // If we have no more children, but used to, they don't line up. + if (!hasChildOrderChanged && prevChildAtSameIndex != null) { + hasChildOrderChanged = true; } } + + if (shouldInclude) { + maybeRecordUpdate(nextFiber, hasChildOrderChanged); + } } function cleanup() { @@ -1002,7 +1063,7 @@ export function attach( }; } - mountFiber(root.current, null); + mountFiberRecursively(root.current, null); flushPendingEvents(root); currentRootID = -1; }); @@ -1013,7 +1074,7 @@ export function attach( // This is not recursive. // We can't traverse fibers after unmounting so instead // we rely on React telling us about each unmount. - enqueueUnmount(fiber); + recordUnmount(fiber); } function handleCommitFiberRoot(root) { @@ -1047,17 +1108,17 @@ export function attach( current.memoizedState != null && current.memoizedState.element != null; if (!wasMounted && isMounted) { // Mount a new root. - mountFiber(current, null); + mountFiberRecursively(current, null); } else if (wasMounted && isMounted) { // Update an existing root. - updateFiber(current, alternate, null); + updateFiberRecursively(current, alternate, null); } else if (wasMounted && !isMounted) { // Unmount an existing root. - enqueueUnmount(current); + recordUnmount(current); } } else { // Mount a new root. - mountFiber(current, null); + mountFiberRecursively(current, null); } if (isProfiling) { diff --git a/src/constants.js b/src/constants.js index ef69e21760..d4ba283319 100644 --- a/src/constants.js +++ b/src/constants.js @@ -4,6 +4,7 @@ export const TREE_OPERATION_ADD = 1; export const TREE_OPERATION_REMOVE = 2; export const TREE_OPERATION_RESET_CHILDREN = 3; export const TREE_OPERATION_UPDATE_TREE_BASE_DURATION = 4; +export const TREE_OPERATION_RECURSIVE_REMOVE_CHILDREN = 5; export const LOCAL_STORAGE_RELOAD_AND_PROFILE_KEY = 'React::DevTools::reloadAndProfile'; diff --git a/src/devtools/store.js b/src/devtools/store.js index fdafcb3f5c..69dbf45133 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -3,6 +3,7 @@ import EventEmitter from 'events'; import { TREE_OPERATION_ADD, + TREE_OPERATION_RECURSIVE_REMOVE_CHILDREN, TREE_OPERATION_REMOVE, TREE_OPERATION_RESET_CHILDREN, TREE_OPERATION_UPDATE_TREE_BASE_DURATION, @@ -572,7 +573,52 @@ export default class Store extends EventEmitter { weightDelta = 1; } break; - case TREE_OPERATION_REMOVE: + case TREE_OPERATION_RECURSIVE_REMOVE_CHILDREN: { + id = ((operations[i + 1]: any): number); + + if (!this._idToElement.has(id)) { + throw new Error( + 'Store does not contain fiber ' + + id + + '. This is a bug in React DevTools.' + ); + } + + i = i + 2; + + let justRemovedIDs = []; + const recursivelyRemove = childID => { + justRemovedIDs.push(childID); + const child = this._idToElement.get(childID); + if (!child) { + throw new Error( + 'Store does not contain fiber ' + + childID + + '. This is a bug in React DevTools.' + ); + } + this._idToElement.delete(childID); + child.children.forEach(recursivelyRemove); + }; + + // Track removed items so search results can be updated + const oldRemovedElementIDs = removedElementIDs; + removedElementIDs = new Uint32Array( + removedElementIDs.length + justRemovedIDs.length + ); + removedElementIDs.set(oldRemovedElementIDs); + let startIndex = oldRemovedElementIDs.length; + for (let j = 0; j < justRemovedIDs.length; j++) { + removedElementIDs[startIndex + j] = oldRemovedElementIDs[j]; + } + + parentElement = ((this._idToElement.get(id): any): Element); + parentElement.children.forEach(recursivelyRemove); + parentElement.children = []; + weightDelta = -parentElement.weight + 1; + break; + } + case TREE_OPERATION_REMOVE: { id = ((operations[i + 1]: any): number); if (!this._idToElement.has(id)) { @@ -614,11 +660,12 @@ export default class Store extends EventEmitter { } // Track removed items so search results can be updated - const oldRemovededElementIDs = removedElementIDs; + const oldRemovedElementIDs = removedElementIDs; removedElementIDs = new Uint32Array(removedElementIDs.length + 1); - removedElementIDs.set(oldRemovededElementIDs); - removedElementIDs[oldRemovededElementIDs.length] = id; + removedElementIDs.set(oldRemovedElementIDs); + removedElementIDs[oldRemovedElementIDs.length] = id; break; + } case TREE_OPERATION_RESET_CHILDREN: id = ((operations[i + 1]: any): number); const numChildren = ((operations[i + 2]: any): number); From 80d9d8d841d02689d4c75360e2b42b543ab3477e Mon Sep 17 00:00:00 2001 From: Dan Date: Fri, 5 Apr 2019 23:34:27 +0100 Subject: [PATCH 4/6] Fix profiler and nits --- shells/dev/app/SuspenseTree/index.js | 10 ++++++---- src/backend/renderer.js | 2 +- .../views/Profiler/CommitTreeBuilder.js | 17 +++++++++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/shells/dev/app/SuspenseTree/index.js b/shells/dev/app/SuspenseTree/index.js index 1e7fec726d..e9a1fef4d2 100644 --- a/shells/dev/app/SuspenseTree/index.js +++ b/shells/dev/app/SuspenseTree/index.js @@ -6,19 +6,21 @@ function SuspenseTree() { return ( <>

Suspense

- +

Primary to Fallback Cycle

+ +

Fallback to Primary Cycle

+ ); } -function PrimaryFallbackTest() { - const [suspend, setSuspend] = useState(false); +function PrimaryFallbackTest({ initialSuspend }) { + const [suspend, setSuspend] = useState(initialSuspend); const fallbackStep = useTestSequence('fallback', Fallback1, Fallback2); const primaryStep = useTestSequence('primary', Primary1, Primary2); return ( <> -

Suspense Primary / Fallback