From 933c664ad6173adda1021ee1cc2d9233c3c13e22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 1 Jul 2019 14:25:07 -0700 Subject: [PATCH] SuspenseList Optimizations (#16005) * Add a bunch of optimizations to SuspenseList We now are able to bail out of reconciliation and splitting out the tail during deep updates that hasn't changed the child props. This only works while the list wasn't suspended before. I also moved the second render of the "head" to the complete phase. This cleans it up a bit for the tail collapsing PR. For this second pass I also use a new technique of resetting the child Fibers for the second pass. This is effectively a fast path to avoid reconciling the children against props again. * Move to didSuspend from SuspenseListState to the effectTag The effectTag now tracks whether the previous commit was suspended. This frees up SuspenseListState to be render-phase only state. We use null to mean the default "independent" mode. * Rename to SuspenseListState to SuspenseListRenderState * Reuse SuspenseListRenderState across render passes * Add optimization to bail out of scanning children if they can't be suspended This optimized the deep update case or initial render without anything suspending. We have some information available to us that tell us if nothing has suspended in the past and nothing has suspended this render pass. This also fixes a bug where we didn't tag the previous render as having suspended boundaries if we didn't need to force a rerender. * rm printChildren oops --- .../react-reconciler/src/ReactChildFiber.js | 13 + packages/react-reconciler/src/ReactFiber.js | 73 ++++ .../src/ReactFiberBeginWork.js | 331 +++++++++--------- .../src/ReactFiberCompleteWork.js | 206 ++++++----- .../src/ReactFiberSuspenseComponent.js | 4 +- .../src/ReactFiberWorkLoop.js | 8 + .../ReactSuspenseList-test.internal.js | 7 +- 7 files changed, 386 insertions(+), 256 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 3d9def376b36..c0aeb7ccdd29 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -33,6 +33,7 @@ import warningWithoutStack from 'shared/warningWithoutStack'; import { createWorkInProgress, + resetWorkInProgress, createFiberFromElement, createFiberFromFragment, createFiberFromText, @@ -1386,3 +1387,15 @@ export function cloneChildFibers( } newChild.sibling = null; } + +// Reset a workInProgress child set to prepare it for a second pass. +export function resetChildFibers( + workInProgress: Fiber, + renderExpirationTime: ExpirationTime, +): void { + let child = workInProgress.child; + while (child !== null) { + resetWorkInProgress(child, renderExpirationTime); + child = child.sibling; + } +} diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 4127967fdc0a..bbaede6bf591 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -481,6 +481,79 @@ export function createWorkInProgress( return workInProgress; } +// Used to reuse a Fiber for a second pass. +export function resetWorkInProgress( + workInProgress: Fiber, + renderExpirationTime: ExpirationTime, +) { + // This resets the Fiber to what createFiber or createWorkInProgress would + // have set the values to before during the first pass. Ideally this wouldn't + // be necessary but unfortunately many code paths reads from the workInProgress + // when they should be reading from current and writing to workInProgress. + + // We assume pendingProps, index, key, ref, return are still untouched to + // avoid doing another reconciliation. + + // Reset the effect tag. + workInProgress.effectTag = NoEffect; + + // The effect list is no longer valid. + workInProgress.nextEffect = null; + workInProgress.firstEffect = null; + workInProgress.lastEffect = null; + + let current = workInProgress.alternate; + if (current === null) { + // Reset to createFiber's initial values. + workInProgress.childExpirationTime = NoWork; + workInProgress.expirationTime = renderExpirationTime; + + workInProgress.child = null; + workInProgress.memoizedProps = null; + workInProgress.memoizedState = null; + workInProgress.updateQueue = null; + + workInProgress.dependencies = null; + + if (enableProfilerTimer) { + // Note: We don't reset the actualTime counts. It's useful to accumulate + // actual time across multiple render passes. + workInProgress.selfBaseDuration = 0; + workInProgress.treeBaseDuration = 0; + } + } else { + // Reset to the cloned values that createWorkInProgress would've. + workInProgress.childExpirationTime = current.childExpirationTime; + workInProgress.expirationTime = current.expirationTime; + + workInProgress.child = current.child; + workInProgress.memoizedProps = current.memoizedProps; + workInProgress.memoizedState = current.memoizedState; + workInProgress.updateQueue = current.updateQueue; + + // Clone the dependencies object. This is mutated during the render phase, so + // it cannot be shared with the current fiber. + const currentDependencies = current.dependencies; + workInProgress.dependencies = + currentDependencies === null + ? null + : { + expirationTime: currentDependencies.expirationTime, + firstContext: currentDependencies.firstContext, + events: currentDependencies.events, + }; + + if (enableProfilerTimer) { + // Note: We don't reset the actualTime counts. It's useful to accumulate + // actual time across multiple render passes. + workInProgress.selfBaseDuration = current.selfBaseDuration; + workInProgress.treeBaseDuration = current.treeBaseDuration; + } + } + + return workInProgress; +} + export function createHostRootFiber(tag: RootTag): Fiber { let mode; if (tag === ConcurrentRoot) { diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index ff9c810be550..09ba304e5a53 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -13,7 +13,7 @@ import type {FiberRoot} from './ReactFiberRoot'; import type {ExpirationTime} from './ReactFiberExpirationTime'; import type { SuspenseState, - SuspenseListState, + SuspenseListRenderState, } from './ReactFiberSuspenseComponent'; import type {SuspenseContext} from './ReactFiberSuspenseContext'; @@ -2007,6 +2007,88 @@ function findLastContentRow(firstChild: null | Fiber): null | Fiber { type SuspenseListRevealOrder = 'forwards' | 'backwards' | 'together' | void; +function validateRevealOrder(revealOrder: SuspenseListRevealOrder) { + if (__DEV__) { + if ( + revealOrder !== undefined && + revealOrder !== 'forwards' && + revealOrder !== 'backwards' && + revealOrder !== 'together' && + !didWarnAboutRevealOrder[revealOrder] + ) { + didWarnAboutRevealOrder[revealOrder] = true; + if (typeof revealOrder === 'string') { + switch (revealOrder.toLowerCase()) { + case 'together': + case 'forwards': + case 'backwards': { + warning( + false, + '"%s" is not a valid value for revealOrder on . ' + + 'Use lowercase "%s" instead.', + revealOrder, + revealOrder.toLowerCase(), + ); + break; + } + case 'forward': + case 'backward': { + warning( + false, + '"%s" is not a valid value for revealOrder on . ' + + 'React uses the -s suffix in the spelling. Use "%ss" instead.', + revealOrder, + revealOrder.toLowerCase(), + ); + break; + } + default: + warning( + false, + '"%s" is not a supported revealOrder on . ' + + 'Did you mean "together", "forwards" or "backwards"?', + revealOrder, + ); + break; + } + } else { + warning( + false, + '%s is not a supported value for revealOrder on . ' + + 'Did you mean "together", "forwards" or "backwards"?', + revealOrder, + ); + } + } + } +} + +function initSuspenseListRenderState( + workInProgress: Fiber, + isBackwards: boolean, + tail: null | Fiber, + lastContentRow: null | Fiber, +): void { + let renderState: null | SuspenseListRenderState = + workInProgress.memoizedState; + if (renderState === null) { + workInProgress.memoizedState = { + isBackwards: isBackwards, + rendering: null, + last: lastContentRow, + tail: tail, + tailExpiration: 0, + }; + } else { + // We can reuse the existing object from previous renders. + renderState.isBackwards = isBackwards; + renderState.rendering = null; + renderState.last = lastContentRow; + renderState.tail = tail; + renderState.tailExpiration = 0; + } +} + // This can end up rendering this component multiple passes. // The first pass splits the children fibers into two sets. A head and tail. // We first render the head. If anything is in fallback state, we do another @@ -2021,24 +2103,11 @@ function updateSuspenseListComponent( ) { const nextProps = workInProgress.pendingProps; const revealOrder: SuspenseListRevealOrder = nextProps.revealOrder; - const nextChildren = nextProps.children; + const newChildren = nextProps.children; - let nextChildFibers; - if (current === null) { - nextChildFibers = mountChildFibers( - workInProgress, - null, - nextChildren, - renderExpirationTime, - ); - } else { - nextChildFibers = reconcileChildFibers( - workInProgress, - current.child, - nextChildren, - renderExpirationTime, - ); - } + validateRevealOrder(revealOrder); + + reconcileChildren(current, workInProgress, newChildren, renderExpirationTime); let suspenseContext: SuspenseContext = suspenseStackCursor.current; @@ -2046,108 +2115,71 @@ function updateSuspenseListComponent( suspenseContext, (ForceSuspenseFallback: SuspenseContext), ); - - if ((workInProgress.effectTag & DidCapture) !== NoEffect) { - // This is the second pass. In this pass, we should force the - // fallbacks in place. - shouldForceFallback = true; - } - - let suspenseListState: null | SuspenseListState = null; - if (shouldForceFallback) { suspenseContext = setShallowSuspenseContext( suspenseContext, ForceSuspenseFallback, ); - suspenseListState = { - didSuspend: true, - isBackwards: false, - rendering: null, - last: null, - tail: null, - tailExpiration: 0, - }; + workInProgress.effectTag |= DidCapture; } else { - let didForceFallback = - current !== null && - current.memoizedState !== null && - (current.memoizedState: SuspenseListState).didSuspend; - if (didForceFallback) { + const didSuspendBefore = + current !== null && (current.effectTag & DidCapture) !== NoEffect; + if (didSuspendBefore) { // If we previously forced a fallback, we need to schedule work // on any nested boundaries to let them know to try to render // again. This is the same as context updating. propagateSuspenseContextChange( workInProgress, - nextChildFibers, + workInProgress.child, renderExpirationTime, ); } suspenseContext = setDefaultShallowSuspenseContext(suspenseContext); } - pushSuspenseContext(workInProgress, suspenseContext); if ((workInProgress.mode & BatchedMode) === NoMode) { + workInProgress.memoizedState = null; + } else { // Outside of batched mode, SuspenseList doesn't work so we just // use make it a noop by treating it as the default revealOrder. - workInProgress.effectTag |= DidCapture; - workInProgress.child = nextChildFibers; - return nextChildFibers; - } - - switch (revealOrder) { - case 'forwards': { - // If need to force fallbacks in this pass we're just going to - // force the whole set to suspend so we don't have to do anything - // further here. - if (!shouldForceFallback) { - let lastContentRow = findLastContentRow(nextChildFibers); + switch (revealOrder) { + case 'forwards': { + let lastContentRow = findLastContentRow(workInProgress.child); let tail; if (lastContentRow === null) { // The whole list is part of the tail. // TODO: We could fast path by just rendering the tail now. - tail = nextChildFibers; - nextChildFibers = null; + tail = workInProgress.child; + workInProgress.child = null; } else { // Disconnect the tail rows after the content row. // We're going to render them separately later. tail = lastContentRow.sibling; lastContentRow.sibling = null; } - if (suspenseListState === null) { - suspenseListState = { - didSuspend: false, - isBackwards: false, - rendering: null, - last: lastContentRow, - tail: tail, - tailExpiration: 0, - }; - } else { - suspenseListState.tail = tail; - } + initSuspenseListRenderState( + workInProgress, + false, // isBackwards + tail, + lastContentRow, + ); + break; } - break; - } - case 'backwards': { - // If need to force fallbacks in this pass we're just going to - // force the whole set to suspend so we don't have to do anything - // further here. - if (!shouldForceFallback) { + case 'backwards': { // We're going to find the first row that has existing content. // At the same time we're going to reverse the list of everything // we pass in the meantime. That's going to be our tail in reverse // order. let tail = null; - let row = nextChildFibers; - nextChildFibers = null; + let row = workInProgress.child; + workInProgress.child = null; while (row !== null) { let currentRow = row.alternate; // New rows can't be content rows. if (currentRow !== null && !isShowingAnyFallbacks(currentRow)) { // This is the beginning of the main content. - nextChildFibers = row; + workInProgress.child = row; break; } let nextRow = row.sibling; @@ -2155,88 +2187,32 @@ function updateSuspenseListComponent( tail = row; row = nextRow; } - // TODO: If nextChildFibers is null, we can continue on the tail immediately. - if (suspenseListState === null) { - suspenseListState = { - didSuspend: false, - isBackwards: true, - rendering: null, - last: null, - tail: tail, - tailExpiration: 0, - }; - } else { - suspenseListState.isBackwards = true; - suspenseListState.tail = tail; - } + // TODO: If workInProgress.child is null, we can continue on the tail immediately. + initSuspenseListRenderState( + workInProgress, + true, // isBackwards + tail, + null, // last + ); + break; } - break; - } - case 'together': { - break; - } - default: { - // The default reveal order is the same as not having - // a boundary. - if (__DEV__) { - if ( - revealOrder !== undefined && - !didWarnAboutRevealOrder[revealOrder] - ) { - didWarnAboutRevealOrder[revealOrder] = true; - if (typeof revealOrder === 'string') { - switch (revealOrder.toLowerCase()) { - case 'together': - case 'forwards': - case 'backwards': { - warning( - false, - '"%s" is not a valid value for revealOrder on . ' + - 'Use lowercase "%s" instead.', - revealOrder, - revealOrder.toLowerCase(), - ); - break; - } - case 'forward': - case 'backward': { - warning( - false, - '"%s" is not a valid value for revealOrder on . ' + - 'React uses the -s suffix in the spelling. Use "%ss" instead.', - revealOrder, - revealOrder.toLowerCase(), - ); - break; - } - default: - warning( - false, - '"%s" is not a supported revealOrder on . ' + - 'Did you mean "together", "forwards" or "backwards"?', - revealOrder, - ); - break; - } - } else { - warning( - false, - '%s is not a supported value for revealOrder on . ' + - 'Did you mean "together", "forwards" or "backwards"?', - revealOrder, - ); - } - } + case 'together': { + initSuspenseListRenderState( + workInProgress, + false, // isBackwards + null, // tail + null, // last + ); + break; + } + default: { + // The default reveal order is the same as not having + // a boundary. + workInProgress.memoizedState = null; } - // We mark this as having captured but it really just says to the - // complete phase that we should treat this as done, whatever form - // it is in. No need for a second pass. - workInProgress.effectTag |= DidCapture; } } - workInProgress.memoizedState = suspenseListState; - workInProgress.child = nextChildFibers; - return nextChildFibers; + return workInProgress.child; } function updatePortalComponent( @@ -2657,19 +2633,46 @@ function beginWork( break; } case SuspenseListComponent: { - // Check if the children have any pending work. + const didSuspendBefore = + (current.effectTag & DidCapture) !== NoEffect; + const childExpirationTime = workInProgress.childExpirationTime; if (childExpirationTime < renderExpirationTime) { + // If none of the children had any work, that means that none of + // them got retried so they'll still be blocked in the same way + // as before. We can fast bail out. pushSuspenseContext(workInProgress, suspenseStackCursor.current); - // None of the children have any work, so we can do a fast bailout. + if (didSuspendBefore) { + workInProgress.effectTag |= DidCapture; + } return null; } - // Try the normal path. - return updateSuspenseListComponent( - current, - workInProgress, - renderExpirationTime, - ); + + if (didSuspendBefore) { + // If something was in fallback state last time, and we have all the + // same children then we're still in progressive loading state. + // Something might get unblocked by state updates or retries in the + // tree which will affect the tail. So we need to use the normal + // path to compute the correct tail. + return updateSuspenseListComponent( + current, + workInProgress, + renderExpirationTime, + ); + } + + // If nothing suspended before and we're rendering the same children, + // then the tail doesn't matter. Anything new that suspends will work + // in the "together" mode, so we can continue from the state we had. + let renderState = workInProgress.memoizedState; + if (renderState !== null) { + // Reset to the "together" mode in case we've started a different + // update in the past but didn't complete it. + renderState.rendering = null; + renderState.tail = null; + } + pushSuspenseContext(workInProgress, suspenseStackCursor.current); + break; } case EventComponent: if (enableFlareAPI) { diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 84ae2cb59e78..a44955c25998 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -20,7 +20,7 @@ import type { import type {ReactEventComponentInstance} from 'shared/ReactTypes'; import type { SuspenseState, - SuspenseListState, + SuspenseListRenderState, } from './ReactFiberSuspenseComponent'; import type {SuspenseContext} from './ReactFiberSuspenseContext'; @@ -114,6 +114,7 @@ import { markSpawnedWork, renderDidSuspend, renderDidSuspendDelayIfPossible, + renderHasNotSuspendedYet, } from './ReactFiberWorkLoop'; import { getEventComponentHostChildrenCount, @@ -122,6 +123,7 @@ import { import getComponentName from 'shared/getComponentName'; import warning from 'shared/warning'; import {Never} from './ReactFiberExpirationTime'; +import {resetChildFibers} from './ReactChildFiber'; function markUpdate(workInProgress: Fiber) { // Tag the fiber with an update effect. This turns a Placement into @@ -553,6 +555,11 @@ function hasSuspendedChildrenAndNewContent( const state: SuspenseState | null = node.memoizedState; const isShowingFallback = state !== null; if (isShowingFallback) { + // Tag the parent fiber as having suspended boundaries. + if (!hasSuspendedBoundaries) { + workInProgress.effectTag |= DidCapture; + } + hasSuspendedBoundaries = true; if (node.updateQueue !== null) { @@ -928,112 +935,139 @@ function completeWork( case SuspenseListComponent: { popSuspenseContext(workInProgress); - if ((workInProgress.effectTag & DidCapture) === NoEffect) { - let suspenseListState: null | SuspenseListState = - workInProgress.memoizedState; - if ( - suspenseListState === null || - suspenseListState.rendering === null - ) { + const renderState: null | SuspenseListRenderState = + workInProgress.memoizedState; + + if (renderState === null) { + // We're running in the default, "independent" mode. We don't do anything + // in this mode. + break; + } + + let didSuspendAlready = + (workInProgress.effectTag & DidCapture) !== NoEffect; + + let renderedTail = renderState.rendering; + if (renderedTail === null) { + // We just rendered the head. + if (!didSuspendAlready) { // This is the first pass. We need to figure out if anything is still // suspended in the rendered set. const renderedChildren = workInProgress.child; // If new content unsuspended, but there's still some content that // didn't. Then we need to do a second pass that forces everything // to keep showing their fallbacks. - const needsRerender = hasSuspendedChildrenAndNewContent( - workInProgress, - renderedChildren, - ); + + // We might be suspended if something in this render pass suspended, or + // something in the previous committed pass suspended. Otherwise, + // there's no chance so we can skip the expensive call to + // hasSuspendedChildrenAndNewContent. + let cannotBeSuspended = + renderHasNotSuspendedYet() && + (current === null || (current.effectTag & DidCapture) === NoEffect); + let needsRerender = + !cannotBeSuspended && + hasSuspendedChildrenAndNewContent(workInProgress, renderedChildren); if (needsRerender) { // Rerender the whole list, but this time, we'll force fallbacks // to stay in place. - workInProgress.effectTag |= DidCapture; // Reset the effect list before doing the second pass since that's now invalid. workInProgress.firstEffect = workInProgress.lastEffect = null; - // Schedule work so we know not to bail out. - workInProgress.expirationTime = renderExpirationTime; - return workInProgress; + // Reset the child fibers to their original state. + resetChildFibers(workInProgress, renderExpirationTime); + + // Set up the Suspense Context to force suspense and immediately + // rerender the children. + pushSuspenseContext( + workInProgress, + setShallowSuspenseContext( + suspenseStackCursor.current, + ForceSuspenseFallback, + ), + ); + return workInProgress.child; } - } else { - // Append the rendered row to the child list. - let rendered = suspenseListState.rendering; - if (!suspenseListState.didSuspend) { - if ( - now() > suspenseListState.tailExpiration && - renderExpirationTime > Never - ) { - // We have now passed our CPU deadline and we'll just give up further - // attempts to render the main content and only render fallbacks. - // The assumption is that this is usually faster. - suspenseListState.didSuspend = true; - // Since nothing actually suspended, there will nothing to ping this - // to get it started back up to attempt the next item. If we can show - // them, then they really have the same priority as this render. - // So we'll pick it back up the very next render pass once we've had - // an opportunity to yield for paint. + // hasSuspendedChildrenAndNewContent could've set didSuspendAlready + didSuspendAlready = + (workInProgress.effectTag & DidCapture) !== NoEffect; + } + // Next we're going to render the tail. + } else { + // Append the rendered row to the child list. + if (!didSuspendAlready) { + if ( + now() > renderState.tailExpiration && + renderExpirationTime > Never + ) { + // We have now passed our CPU deadline and we'll just give up further + // attempts to render the main content and only render fallbacks. + // The assumption is that this is usually faster. + workInProgress.effectTag |= DidCapture; + didSuspendAlready = true; + // Since nothing actually suspended, there will nothing to ping this + // to get it started back up to attempt the next item. If we can show + // them, then they really have the same priority as this render. + // So we'll pick it back up the very next render pass once we've had + // an opportunity to yield for paint. - const nextPriority = renderExpirationTime - 1; - workInProgress.expirationTime = workInProgress.childExpirationTime = nextPriority; - if (enableSchedulerTracing) { - markSpawnedWork(nextPriority); - } - } else { - suspenseListState.didSuspend = isShowingAnyFallbacks(rendered); + const nextPriority = renderExpirationTime - 1; + workInProgress.expirationTime = workInProgress.childExpirationTime = nextPriority; + if (enableSchedulerTracing) { + markSpawnedWork(nextPriority); } + } else if (isShowingAnyFallbacks(renderedTail)) { + workInProgress.effectTag |= DidCapture; + didSuspendAlready = true; } - if (suspenseListState.isBackwards) { - // The effect list of the backwards tail will have been added - // to the end. This breaks the guarantee that life-cycles fire in - // sibling order but that isn't a strong guarantee promised by React. - // Especially since these might also just pop in during future commits. - // Append to the beginning of the list. - rendered.sibling = workInProgress.child; - workInProgress.child = rendered; + } + if (renderState.isBackwards) { + // The effect list of the backwards tail will have been added + // to the end. This breaks the guarantee that life-cycles fire in + // sibling order but that isn't a strong guarantee promised by React. + // Especially since these might also just pop in during future commits. + // Append to the beginning of the list. + renderedTail.sibling = workInProgress.child; + workInProgress.child = renderedTail; + } else { + let previousSibling = renderState.last; + if (previousSibling !== null) { + previousSibling.sibling = renderedTail; } else { - let previousSibling = suspenseListState.last; - if (previousSibling !== null) { - previousSibling.sibling = rendered; - } else { - workInProgress.child = rendered; - } - suspenseListState.last = rendered; + workInProgress.child = renderedTail; } + renderState.last = renderedTail; } + } - if (suspenseListState !== null && suspenseListState.tail !== null) { - // We still have tail rows to render. - if (suspenseListState.tailExpiration === 0) { - // Heuristic for how long we're willing to spend rendering rows - // until we just give up and show what we have so far. - const TAIL_EXPIRATION_TIMEOUT_MS = 500; - suspenseListState.tailExpiration = - now() + TAIL_EXPIRATION_TIMEOUT_MS; - } - // Pop a row. - let next = suspenseListState.tail; - suspenseListState.rendering = next; - suspenseListState.tail = next.sibling; - next.sibling = null; + if (renderState.tail !== null) { + // We still have tail rows to render. + if (renderState.tailExpiration === 0) { + // Heuristic for how long we're willing to spend rendering rows + // until we just give up and show what we have so far. + const TAIL_EXPIRATION_TIMEOUT_MS = 500; + renderState.tailExpiration = now() + TAIL_EXPIRATION_TIMEOUT_MS; + } + // Pop a row. + let next = renderState.tail; + renderState.rendering = next; + renderState.tail = next.sibling; + next.sibling = null; - // Restore the context. - // TODO: We can probably just avoid popping it instead and only - // setting it the first time we go from not suspended to suspended. - let suspenseContext = suspenseStackCursor.current; - if (suspenseListState.didSuspend) { - suspenseContext = setShallowSuspenseContext( - suspenseContext, - ForceSuspenseFallback, - ); - } else { - suspenseContext = setDefaultShallowSuspenseContext(suspenseContext); - } - pushSuspenseContext(workInProgress, suspenseContext); - // Do a pass over the next row. - return next; + // Restore the context. + // TODO: We can probably just avoid popping it instead and only + // setting it the first time we go from not suspended to suspended. + let suspenseContext = suspenseStackCursor.current; + if (didSuspendAlready) { + suspenseContext = setShallowSuspenseContext( + suspenseContext, + ForceSuspenseFallback, + ); + } else { + suspenseContext = setDefaultShallowSuspenseContext(suspenseContext); } - } else { - workInProgress.effectTag &= ~DidCapture; + pushSuspenseContext(workInProgress, suspenseContext); + // Do a pass over the next row. + return next; } break; } diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index c56c923fd8f1..8ff34224c618 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -11,10 +11,10 @@ import type {Fiber} from './ReactFiber'; import {SuspenseComponent} from 'shared/ReactWorkTags'; // TODO: This is now an empty object. Should we switch this to a boolean? +// Alternatively we can make this use an effect tag similar to SuspenseList. export type SuspenseState = {||}; -export type SuspenseListState = {| - didSuspend: boolean, +export type SuspenseListRenderState = {| isBackwards: boolean, // The currently rendering tail row. rendering: null | Fiber, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 1308452a7785..c05717bfe3ec 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -1201,6 +1201,14 @@ export function renderDidError() { } } +// Called during render to determine if anything has suspended. +// Returns false if we're not sure. +export function renderHasNotSuspendedYet(): boolean { + // If something errored or completed, we can't really be sure, + // so those are false. + return workInProgressRootExitStatus === RootIncomplete; +} + function inferTimeFromExpirationTime(expirationTime: ExpirationTime): number { // We don't know exactly when the update was scheduled, but we can infer an // approximate start time from the expiration time. diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js index b028734baba1..1ad4a9c9faa0 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js @@ -578,7 +578,7 @@ describe('ReactSuspenseList', () => { await B.resolve(); - expect(Scheduler).toFlushAndYield(['B']); + expect(Scheduler).toFlushAndYield(['B', 'Suspend! [C]']); // Even though we could now show B, we're still waiting on C. expect(ReactNoop).toMatchRenderedOutput( @@ -780,7 +780,6 @@ describe('ReactSuspenseList', () => { 'Suspend! [C]', 'Loading C', 'D', - 'Suspend! [E]', 'Loading E', 'Loading F', ]); @@ -861,7 +860,7 @@ describe('ReactSuspenseList', () => { ); }); - it('displays added row at the top "together" and the bottom in "forwards" order', async () => { + it('displays added row at the top "together" and the bottom in "backwards" order', async () => { let A = createAsyncText('A'); let B = createAsyncText('B'); let D = createAsyncText('D'); @@ -958,7 +957,7 @@ describe('ReactSuspenseList', () => { await F.resolve(); - expect(Scheduler).toFlushAndYield(['F']); + expect(Scheduler).toFlushAndYield(['Suspend! [D]', 'F']); // Even though we could show F, it is still in a fallback state because // E is not yet resolved. We need to resolve everything in the head first.