From a7a5a3343c03f7d96eb244af04e08ce40a328707 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 5 Nov 2018 16:32:50 -0800 Subject: [PATCH] [suspense] Avoid double commit by re-rendering immediately and reusing primary children (#14083) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Avoid double commit by re-rendering immediately and reusing children To support Suspense outside of concurrent mode, any component that starts rendering must commit synchronously without being interrupted. This means normal path, where we unwind the stack and try again from the nearest Suspense boundary, won't work. We used to have a special case where we commit the suspended tree in an incomplete state. Then, in a subsequent commit, we re-render using the fallback. The first part — committing an incomplete tree — hasn't changed with this PR. But I've changed the second part — now we render the fallback children immediately, within the same commit. * Add a failing test for remounting fallback in sync mode * Add failing test for stuck Suspense fallback * Toggle visibility of Suspense children in mutation phase, not layout If parent reads visibility of children in a lifecycle, they should have already updated. --- ...actDOMSuspensePlaceholder-test.internal.js | 49 ++++++ .../src/ReactFiberBeginWork.js | 154 ++++++++++++----- .../src/ReactFiberCommitWork.js | 71 +++----- .../src/ReactFiberCompleteWork.js | 44 +++-- .../src/ReactFiberScheduler.js | 6 + .../src/ReactFiberSuspenseComponent.js | 13 +- .../src/ReactFiberUnwindWork.js | 38 +---- .../__tests__/ReactSuspense-test.internal.js | 156 +++++++++++++++++- ...tSuspenseWithNoopRenderer-test.internal.js | 72 +++++--- .../__tests__/ReactProfiler-test.internal.js | 7 +- 10 files changed, 432 insertions(+), 178 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.internal.js index 5c35263789743..fbed0d4b82a75 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.internal.js @@ -9,6 +9,7 @@ 'use strict'; +let ReactFeatureFlags; let React; let ReactDOM; let Suspense; @@ -20,6 +21,8 @@ describe('ReactDOMSuspensePlaceholder', () => { beforeEach(() => { jest.resetModules(); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.enableHooks = true; React = require('react'); ReactDOM = require('react-dom'); ReactCache = require('react-cache'); @@ -108,4 +111,50 @@ describe('ReactDOMSuspensePlaceholder', () => { expect(container.textContent).toEqual('ABC'); }); + + it( + 'outside concurrent mode, re-hides children if their display is updated ' + + 'but the boundary is still showing the fallback', + async () => { + const {useState} = React; + + let setIsVisible; + function Sibling({children}) { + const [isVisible, _setIsVisible] = useState(false); + setIsVisible = _setIsVisible; + return ( + + {children} + + ); + } + + function App() { + return ( + }> + Sibling + + + + + ); + } + + ReactDOM.render(, container); + expect(container.innerHTML).toEqual( + 'SiblingLoading...', + ); + + setIsVisible(true); + expect(container.innerHTML).toEqual( + 'SiblingLoading...', + ); + + await advanceTimers(1000); + + expect(container.innerHTML).toEqual( + 'SiblingAsync', + ); + }, + ); }); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 92f4ccef7954e..67808381bf06a 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -66,7 +66,7 @@ import { } from './ReactChildFiber'; import {processUpdateQueue} from './ReactUpdateQueue'; import {NoWork, Never} from './ReactFiberExpirationTime'; -import {ConcurrentMode, StrictMode} from './ReactTypeOfMode'; +import {ConcurrentMode, StrictMode, NoContext} from './ReactTypeOfMode'; import { shouldSetTextContent, shouldDeprioritizeSubtree, @@ -1071,31 +1071,21 @@ function updateSuspenseComponent( // We should attempt to render the primary children unless this boundary // already suspended during this render (`alreadyCaptured` is true). let nextState: SuspenseState | null = workInProgress.memoizedState; - if (nextState === null) { - // An empty suspense state means this boundary has not yet timed out. + + let nextDidTimeout; + if ((workInProgress.effectTag & DidCapture) === NoEffect) { + // This is the first attempt. + nextState = null; + nextDidTimeout = false; } else { - if (!nextState.alreadyCaptured) { - // Since we haven't already suspended during this commit, clear the - // existing suspense state. We'll try rendering again. - nextState = null; - } else { - // Something in this boundary's subtree already suspended. Switch to - // rendering the fallback children. Set `alreadyCaptured` to true. - if (current !== null && nextState === current.memoizedState) { - // Create a new suspense state to avoid mutating the current tree's. - nextState = { - alreadyCaptured: true, - didTimeout: true, - timedOutAt: nextState.timedOutAt, - }; - } else { - // Already have a clone, so it's safe to mutate. - nextState.alreadyCaptured = true; - nextState.didTimeout = true; - } - } + // Something in this boundary's subtree already suspended. Switch to + // rendering the fallback children. + nextState = { + timedOutAt: nextState !== null ? nextState.timedOutAt : NoWork, + }; + nextDidTimeout = true; + workInProgress.effectTag &= ~DidCapture; } - const nextDidTimeout = nextState !== null && nextState.didTimeout; // This next part is a bit confusing. If the children timeout, we switch to // showing the fallback children in place of the "primary" children. @@ -1140,6 +1130,22 @@ function updateSuspenseComponent( NoWork, null, ); + + if ((workInProgress.mode & ConcurrentMode) === NoContext) { + // Outside of concurrent mode, we commit the effects from the + // partially completed, timed-out tree, too. + const progressedState: SuspenseState = workInProgress.memoizedState; + const progressedPrimaryChild: Fiber | null = + progressedState !== null + ? (workInProgress.child: any).child + : (workInProgress.child: any); + reuseProgressedPrimaryChild( + workInProgress, + primaryChildFragment, + progressedPrimaryChild, + ); + } + const fallbackChildFragment = createFiberFromFragment( nextFallbackChildren, mode, @@ -1166,7 +1172,7 @@ function updateSuspenseComponent( // This is an update. This branch is more complicated because we need to // ensure the state of the primary children is preserved. const prevState = current.memoizedState; - const prevDidTimeout = prevState !== null && prevState.didTimeout; + const prevDidTimeout = prevState !== null; if (prevDidTimeout) { // The current tree already timed out. That means each child set is // wrapped in a fragment fiber. @@ -1182,6 +1188,24 @@ function updateSuspenseComponent( NoWork, ); primaryChildFragment.effectTag |= Placement; + + if ((workInProgress.mode & ConcurrentMode) === NoContext) { + // Outside of concurrent mode, we commit the effects from the + // partially completed, timed-out tree, too. + const progressedState: SuspenseState = workInProgress.memoizedState; + const progressedPrimaryChild: Fiber | null = + progressedState !== null + ? (workInProgress.child: any).child + : (workInProgress.child: any); + if (progressedPrimaryChild !== currentPrimaryChildFragment.child) { + reuseProgressedPrimaryChild( + workInProgress, + primaryChildFragment, + progressedPrimaryChild, + ); + } + } + // Clone the fallback child fragment, too. These we'll continue // working on. const fallbackChildFragment = (primaryChildFragment.sibling = createWorkInProgress( @@ -1237,6 +1261,22 @@ function updateSuspenseComponent( primaryChildFragment.effectTag |= Placement; primaryChildFragment.child = currentPrimaryChild; currentPrimaryChild.return = primaryChildFragment; + + if ((workInProgress.mode & ConcurrentMode) === NoContext) { + // Outside of concurrent mode, we commit the effects from the + // partially completed, timed-out tree, too. + const progressedState: SuspenseState = workInProgress.memoizedState; + const progressedPrimaryChild: Fiber | null = + progressedState !== null + ? (workInProgress.child: any).child + : (workInProgress.child: any); + reuseProgressedPrimaryChild( + workInProgress, + primaryChildFragment, + progressedPrimaryChild, + ); + } + // Create a fragment from the fallback children, too. const fallbackChildFragment = (primaryChildFragment.sibling = createFiberFromFragment( nextFallbackChildren, @@ -1270,6 +1310,49 @@ function updateSuspenseComponent( return next; } +function reuseProgressedPrimaryChild( + workInProgress: Fiber, + primaryChildFragment: Fiber, + progressedChild: Fiber | null, +) { + // This function is only called outside concurrent mode. Usually, if a work- + // in-progress primary tree suspends, we throw it out and revert back to + // current. Outside concurrent mode, though, we commit the suspended work-in- + // progress, even though it didn't complete. This function reuses the children + // and transfers the effects. + let child = (primaryChildFragment.child = progressedChild); + while (child !== null) { + // Ensure that the first and last effect of the parent corresponds + // to the children's first and last effect. + if (primaryChildFragment.firstEffect === null) { + primaryChildFragment.firstEffect = child.firstEffect; + } + if (child.lastEffect !== null) { + if (primaryChildFragment.lastEffect !== null) { + primaryChildFragment.lastEffect.nextEffect = child.firstEffect; + } + primaryChildFragment.lastEffect = child.lastEffect; + } + + // Append all the effects of the subtree and this fiber onto the effect + // list of the parent. The completion order of the children affects the + // side-effect order. + if (child.effectTag > PerformedWork) { + if (primaryChildFragment.lastEffect !== null) { + primaryChildFragment.lastEffect.nextEffect = child; + } else { + primaryChildFragment.firstEffect = child; + } + primaryChildFragment.lastEffect = child; + } + child.return = primaryChildFragment; + child = child.sibling; + } + + workInProgress.firstEffect = primaryChildFragment.firstEffect; + workInProgress.lastEffect = primaryChildFragment.lastEffect; +} + function updatePortalComponent( current: Fiber | null, workInProgress: Fiber, @@ -1426,25 +1509,6 @@ function updateContextConsumer( return workInProgress.child; } -/* - function reuseChildrenEffects(returnFiber : Fiber, firstChild : Fiber) { - let child = firstChild; - do { - // Ensure that the first and last effect of the parent corresponds - // to the children's first and last effect. - if (!returnFiber.firstEffect) { - returnFiber.firstEffect = child.firstEffect; - } - if (child.lastEffect) { - if (returnFiber.lastEffect) { - returnFiber.lastEffect.nextEffect = child.firstEffect; - } - returnFiber.lastEffect = child.lastEffect; - } - } while (child = child.sibling); - } - */ - function bailoutOnAlreadyFinishedWork( current: Fiber | null, workInProgress: Fiber, @@ -1528,7 +1592,7 @@ function beginWork( break; case SuspenseComponent: { const state: SuspenseState | null = workInProgress.memoizedState; - const didTimeout = state !== null && state.didTimeout; + const didTimeout = state !== null; if (didTimeout) { // If this boundary is currently timed out, we need to decide // whether to retry the primary children, or to skip over it and diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index b8e4ac203b66b..138267f3fdd91 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -50,13 +50,12 @@ import { Placement, Snapshot, Update, - Callback, } from 'shared/ReactSideEffectTags'; import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; import warningWithoutStack from 'shared/warningWithoutStack'; -import {NoWork, Sync} from './ReactFiberExpirationTime'; +import {NoWork} from './ReactFiberExpirationTime'; import {onCommitUnmount} from './ReactFiberDevToolsHook'; import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf'; import {getStackByFiberInDevAndProd} from './ReactCurrentFiber'; @@ -86,9 +85,7 @@ import { } from './ReactFiberHostConfig'; import { captureCommitPhaseError, - flushPassiveEffects, requestCurrentTime, - scheduleWork, } from './ReactFiberScheduler'; import { NoEffect as NoHookEffect, @@ -452,50 +449,8 @@ function commitLifeCycles( } return; } - case SuspenseComponent: { - if (finishedWork.effectTag & Callback) { - // In non-strict mode, a suspense boundary times out by commiting - // twice: first, by committing the children in an inconsistent state, - // then hiding them and showing the fallback children in a subsequent - // commit. - const newState: SuspenseState = { - alreadyCaptured: true, - didTimeout: false, - timedOutAt: NoWork, - }; - finishedWork.memoizedState = newState; - flushPassiveEffects(); - scheduleWork(finishedWork, Sync); - return; - } - let oldState: SuspenseState | null = - current !== null ? current.memoizedState : null; - let newState: SuspenseState | null = finishedWork.memoizedState; - let oldDidTimeout = oldState !== null ? oldState.didTimeout : false; - - let newDidTimeout; - let primaryChildParent = finishedWork; - if (newState === null) { - newDidTimeout = false; - } else { - newDidTimeout = newState.didTimeout; - if (newDidTimeout) { - primaryChildParent = finishedWork.child; - newState.alreadyCaptured = false; - if (newState.timedOutAt === NoWork) { - // If the children had not already timed out, record the time. - // This is used to compute the elapsed time during subsequent - // attempts to render the children. - newState.timedOutAt = requestCurrentTime(); - } - } - } - - if (newDidTimeout !== oldDidTimeout && primaryChildParent !== null) { - hideOrUnhideAllChildren(primaryChildParent, newDidTimeout); - } - return; - } + case SuspenseComponent: + break; case IncompleteClassComponent: break; default: { @@ -1066,6 +1021,26 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { return; } case SuspenseComponent: { + let newState: SuspenseState | null = finishedWork.memoizedState; + + let newDidTimeout; + let primaryChildParent = finishedWork; + if (newState === null) { + newDidTimeout = false; + } else { + newDidTimeout = true; + primaryChildParent = finishedWork.child; + if (newState.timedOutAt === NoWork) { + // If the children had not already timed out, record the time. + // This is used to compute the elapsed time during subsequent + // attempts to render the children. + newState.timedOutAt = requestCurrentTime(); + } + } + + if (primaryChildParent !== null) { + hideOrUnhideAllChildren(primaryChildParent, newDidTimeout); + } return; } case IncompleteClassComponent: { diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 79c36c1a7747d..d435d3296aeab 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -39,7 +39,13 @@ import { LazyComponent, IncompleteClassComponent, } from 'shared/ReactWorkTags'; -import {Placement, Ref, Update} from 'shared/ReactSideEffectTags'; +import { + Placement, + Ref, + Update, + NoEffect, + DidCapture, +} from 'shared/ReactSideEffectTags'; import invariant from 'shared/invariant'; import { @@ -75,6 +81,7 @@ import { prepareToHydrateHostTextInstance, popHydrationState, } from './ReactFiberHydrationContext'; +import {ConcurrentMode, NoContext} from './ReactTypeOfMode'; function markUpdate(workInProgress: Fiber) { // Tag the fiber with an update effect. This turns a Placement into @@ -247,8 +254,8 @@ if (supportsMutation) { if (current !== null) { const oldState: SuspenseState = current.memoizedState; const newState: SuspenseState = node.memoizedState; - const oldIsHidden = oldState !== null && oldState.didTimeout; - const newIsHidden = newState !== null && newState.didTimeout; + const oldIsHidden = oldState !== null; + const newIsHidden = newState !== null; if (oldIsHidden !== newIsHidden) { // The placeholder either just timed out or switched back to the normal // children after having previously timed out. Toggle the visibility of @@ -350,8 +357,8 @@ if (supportsMutation) { if (current !== null) { const oldState: SuspenseState = current.memoizedState; const newState: SuspenseState = node.memoizedState; - const oldIsHidden = oldState !== null && oldState.didTimeout; - const newIsHidden = newState !== null && newState.didTimeout; + const oldIsHidden = oldState !== null; + const newIsHidden = newState !== null; if (oldIsHidden !== newIsHidden) { // The placeholder either just timed out or switched back to the normal // children after having previously timed out. Toggle the visibility of @@ -690,12 +697,27 @@ function completeWork( break; case SuspenseComponent: { const nextState = workInProgress.memoizedState; - const prevState = current !== null ? current.memoizedState : null; - const nextDidTimeout = nextState !== null && nextState.didTimeout; - const prevDidTimeout = prevState !== null && prevState.didTimeout; - if (nextDidTimeout !== prevDidTimeout) { - // If this render commits, and it switches between the normal state - // and the timed-out state, schedule an effect. + if ((workInProgress.effectTag & DidCapture) !== NoEffect) { + // Something suspended. Re-render with the fallback children. + workInProgress.expirationTime = renderExpirationTime; + workInProgress.firstEffect = workInProgress.lastEffect = null; + return workInProgress; + } + + const nextDidTimeout = nextState !== null; + const prevDidTimeout = current !== null && current.memoizedState !== null; + // The children either timed out after previously being visible, or + // were restored after previously being hidden. Schedule an effect + // to update their visiblity. + if ( + // + nextDidTimeout !== prevDidTimeout || + // Outside concurrent mode, the primary children commit in an + // inconsistent state, even if they are hidden. So if they are hidden, + // we need to schedule an effect to re-hide them, just in case. + ((workInProgress.effectTag & ConcurrentMode) === NoContext && + nextDidTimeout) + ) { workInProgress.effectTag |= Update; } break; diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 75717b03d7d17..719a0bdef8a97 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -981,6 +981,12 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null { ReactCurrentFiber.resetCurrentFiber(); } + if (nextUnitOfWork !== null) { + // Completing this fiber spawned new work. Work on that next. + nextUnitOfWork.firstEffect = nextUnitOfWork.lastEffect = null; + return nextUnitOfWork; + } + if ( returnFiber !== null && // Do not append effects to parents if a sibling failed to complete diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index 0ff04687e3ba3..c31cf93473928 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -11,17 +11,6 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; export type SuspenseState = {| - // Whether a component in the child subtree already suspended. If true, - // subsequent suspends should bubble up to the next boundary. - alreadyCaptured: boolean, - // Whether the boundary renders the primary or fallback children. This is - // separate from `alreadyCaptured` because outside of strict mode, when a - // boundary times out, the first commit renders the primary children in an - // incomplete state, then performs a second commit to switch the fallback. - // In that first commit, `alreadyCaptured` is false and `didTimeout` is true. - didTimeout: boolean, - // The time at which the boundary timed out. This is separate from - // `didTimeout` because it's not set unless the boundary actually commits. timedOutAt: ExpirationTime, |}; @@ -36,5 +25,5 @@ export function shouldCaptureSuspense( // If it was the primary children that just suspended, capture and render the // fallback. Otherwise, don't capture and bubble to the next boundary. const nextState: SuspenseState | null = workInProgress.memoizedState; - return nextState === null || !nextState.didTimeout; + return nextState === null; } diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 004d5e5f6ab30..6fb2e080687e5 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -32,7 +32,6 @@ import { Incomplete, NoEffect, ShouldCapture, - Callback as CallbackEffect, LifecycleEffectMask, } from 'shared/ReactSideEffectTags'; import {enableSchedulerTracing} from 'shared/ReactFeatureFlags'; @@ -66,7 +65,6 @@ import { import invariant from 'shared/invariant'; import maxSigned31BitInt from './maxSigned31BitInt'; import { - NoWork, Sync, expirationTimeToMs, LOW_PRIORITY_EXPIRATION, @@ -176,7 +174,7 @@ function throwException( const current = workInProgress.alternate; if (current !== null) { const currentState: SuspenseState | null = current.memoizedState; - if (currentState !== null && currentState.didTimeout) { + if (currentState !== null) { // Reached a boundary that already timed out. Do not search // any further. const timedOutAt = currentState.timedOutAt; @@ -238,7 +236,7 @@ function throwException( // inside a concurrent mode tree. If the Suspense is outside of it, we // should *not* suspend the commit. if ((workInProgress.mode & ConcurrentMode) === NoEffect) { - workInProgress.effectTag |= CallbackEffect; + workInProgress.effectTag |= DidCapture; // Unmount the source fiber's children const nextChildren = null; @@ -265,6 +263,10 @@ function throwException( } } + // The source fiber did not complete. Mark it with the current + // render priority to indicate that it still has pending work. + sourceFiber.expirationTime = renderExpirationTime; + // Exit without suspending. return; } @@ -415,33 +417,7 @@ function unwindWork( const effectTag = workInProgress.effectTag; if (effectTag & ShouldCapture) { workInProgress.effectTag = (effectTag & ~ShouldCapture) | DidCapture; - // Captured a suspense effect. Set the boundary's `alreadyCaptured` - // state to true so we know to render the fallback. - const current = workInProgress.alternate; - const currentState: SuspenseState | null = - current !== null ? current.memoizedState : null; - let nextState: SuspenseState | null = workInProgress.memoizedState; - if (nextState === null) { - // No existing state. Create a new object. - nextState = { - alreadyCaptured: true, - didTimeout: false, - timedOutAt: NoWork, - }; - } else if (currentState === nextState) { - // There is an existing state but it's the same as the current tree's. - // Clone the object. - nextState = { - alreadyCaptured: true, - didTimeout: nextState.didTimeout, - timedOutAt: nextState.timedOutAt, - }; - } else { - // Already have a clone, so it's safe to mutate. - nextState.alreadyCaptured = true; - } - workInProgress.memoizedState = nextState; - // Re-render the boundary. + // Captured a suspense effect. Re-render the boundary. return workInProgress; } return null; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index ac32642985165..37fd4c70d0a7f 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -401,14 +401,12 @@ describe('ReactSuspense', () => { 'A', 'Suspend! [B:1]', 'C', + 'Loading...', 'Mount [A]', // B's lifecycle should not fire because it suspended // 'Mount [B]', 'Mount [C]', - - // In a subsequent commit, render a placeholder - 'Loading...', 'Mount [Loading...]', ]); expect(root).toMatchRenderedOutput('Loading...'); @@ -483,13 +481,76 @@ describe('ReactSuspense', () => { expect(root).toMatchRenderedOutput('Loading...'); instance.setState({step: 2}); + expect(ReactTestRenderer).toHaveYielded(['Stateful: 2', 'Suspend! [B]']); + expect(root).toMatchRenderedOutput('Loading...'); + + jest.advanceTimersByTime(1000); expect(ReactTestRenderer).toHaveYielded([ - 'Stateful: 2', + 'Promise resolved [B]', + 'B', + 'B', + ]); + expect(root).toMatchRenderedOutput('Stateful: 2B'); + }); + + it('when updating a timed-out tree, always retries the suspended component', () => { + let instance; + class Stateful extends React.Component { + state = {step: 1}; + render() { + instance = this; + return ; + } + } + + const Indirection = React.Fragment; + + function App(props) { + return ( + }> + + + + + + + + + + ); + } + + const root = ReactTestRenderer.create(); + + expect(ReactTestRenderer).toHaveYielded([ + 'Stateful: 1', + 'Suspend! [A]', + 'Loading...', + ]); + + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded(['Promise resolved [A]', 'A']); + expect(root).toMatchRenderedOutput('Stateful: 1A'); + + root.update(); + expect(ReactTestRenderer).toHaveYielded([ + 'Stateful: 1', 'Suspend! [B]', 'Loading...', ]); expect(root).toMatchRenderedOutput('Loading...'); + instance.setState({step: 2}); + expect(ReactTestRenderer).toHaveYielded([ + 'Stateful: 2', + + // The suspended component should suspend again. If it doesn't, the + // likely mistake is that the suspended fiber wasn't marked with + // pending work, so it was improperly treated as complete. + 'Suspend! [B]', + ]); + expect(root).toMatchRenderedOutput('Loading...'); + jest.advanceTimersByTime(1000); expect(ReactTestRenderer).toHaveYielded([ 'Promise resolved [B]', @@ -631,5 +692,92 @@ describe('ReactSuspense', () => { expect(root).toFlushAndYield(['Step: 3']); expect(root).toMatchRenderedOutput('Step: 3'); }); + + it('does not remount the fallback while suspended children resolve in sync mode', () => { + let mounts = 0; + class ShouldMountOnce extends React.Component { + componentDidMount() { + mounts++; + } + render() { + return ; + } + } + + function App(props) { + return ( + }> + + + + + ); + } + + const root = ReactTestRenderer.create(); + + // Initial render + expect(ReactTestRenderer).toHaveYielded([ + 'Suspend! [Child 1]', + 'Suspend! [Child 2]', + 'Suspend! [Child 3]', + 'Loading...', + ]); + expect(root).toFlushAndYield([]); + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [Child 1]', + 'Child 1', + 'Suspend! [Child 2]', + 'Suspend! [Child 3]', + ]); + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [Child 2]', + 'Child 2', + 'Suspend! [Child 3]', + // TODO: This suspends twice because there were two pings, once for each + // time Child 2 suspended. This is only an issue in sync mode because + // pings aren't batched. + 'Suspend! [Child 3]', + ]); + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded([ + 'Promise resolved [Child 3]', + 'Child 3', + ]); + expect(root).toMatchRenderedOutput( + ['Child 1', 'Child 2', 'Child 3'].join(''), + ); + expect(mounts).toBe(1); + }); + + it('does not get stuck with fallback in concurrent mode for a large delay', () => { + function App(props) { + return ( + }> + + + + ); + } + + const root = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); + + expect(root).toFlushAndYield([ + 'Suspend! [Child 1]', + 'Suspend! [Child 2]', + 'Loading...', + ]); + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded(['Promise resolved [Child 1]']); + expect(root).toFlushAndYield(['Child 1', 'Suspend! [Child 2]']); + jest.advanceTimersByTime(6000); + expect(ReactTestRenderer).toHaveYielded(['Promise resolved [Child 2]']); + expect(root).toFlushAndYield(['Child 1', 'Child 2']); + expect(root).toMatchRenderedOutput(['Child 1', 'Child 2'].join('')); + }); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 3d02a0084395b..de4b75337ef3f 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -17,6 +17,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { beforeEach(() => { jest.resetModules(); ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.enableHooks = true; ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; React = require('react'); @@ -929,11 +930,10 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Suspend during an async render. expect(ReactNoop.flushNextYield()).toEqual(['Suspend! [Step: 2]']); expect(ReactNoop.flush()).toEqual([ - 'Update did commit', - // Switch to the placeholder in a subsequent commit 'Loading (1)', 'Loading (2)', 'Loading (3)', + 'Update did commit', ]); expect(ReactNoop.getChildrenAsJSX()).toEqual( @@ -1012,12 +1012,11 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Before', 'Suspend! [Async: 1]', 'After', + 'Loading...', 'Before', 'Sync: 1', 'After', 'Did mount', - // The placeholder is rendered in a subsequent commit - 'Loading...', 'Promise resolved [Async: 1]', 'Async: 1', ]); @@ -1051,14 +1050,12 @@ describe('ReactSuspenseWithNoopRenderer', () => { ]); expect(ReactNoop.flush()).toEqual([ 'After', + 'Loading...', 'Before', 'Sync: 2', 'After', 'Update 1 did commit', 'Update 2 did commit', - - // Switch to the placeholder in a subsequent commit - 'Loading...', ]); expect(ReactNoop.getChildrenAsJSX()).toEqual( @@ -1149,12 +1146,11 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Before', 'Suspend! [Async: 1]', 'After', + 'Loading...', 'Before', 'Sync: 1', 'After', 'Did mount', - // The placeholder is rendered in a subsequent commit - 'Loading...', 'Promise resolved [Async: 1]', 'Async: 1', ]); @@ -1188,14 +1184,12 @@ describe('ReactSuspenseWithNoopRenderer', () => { ]); expect(ReactNoop.flush()).toEqual([ 'After', + 'Loading...', 'Before', 'Sync: 2', 'After', 'Update 1 did commit', 'Update 2 did commit', - - // Switch to the placeholder in a subsequent commit - 'Loading...', ]); expect(ReactNoop.getChildrenAsJSX()).toEqual( @@ -1276,16 +1270,13 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Suspend! [B]', 'C', + 'Loading...', 'Mount [A]', 'Mount [B]', 'Mount [C]', - 'Commit root', - - // In a subsequent commit, render a placeholder - 'Loading...', - // Force delete all the existing children when switching to the - // placeholder. This should be a mount, not an update. + // This should be a mount, not an update. 'Mount [Loading...]', + 'Commit root', ]); expect(ReactNoop.getChildrenAsJSX()).toEqual( @@ -1388,6 +1379,45 @@ describe('ReactSuspenseWithNoopRenderer', () => { await advanceTimers(100); expect(ReactNoop.getChildren()).toEqual([span('Hi')]); }); + + it('toggles visibility during the mutation phase', async () => { + const {useRef, useLayoutEffect} = React; + + function Parent() { + const child = useRef(null); + + useLayoutEffect(() => { + ReactNoop.yield('Child is hidden: ' + child.current.hidden); + }); + + return ( + + ); + } + + function App(props) { + return ( + }> + + + ); + } + + ReactNoop.renderLegacySyncRoot(); + + expect(ReactNoop.clearYields()).toEqual([ + 'Suspend! [Hi]', + 'Loading...', + // The child should have already been hidden + 'Child is hidden: true', + ]); + + await advanceTimers(1000); + + expect(ReactNoop.clearYields()).toEqual(['Promise resolved [Hi]', 'Hi']); + }); }); it('does not call lifecycles of a suspended component', async () => { @@ -1453,16 +1483,14 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'A', 'Suspend! [B]', 'C', + 'Loading...', 'Mount [A]', // B's lifecycle should not fire because it suspended // 'Mount [B]', 'Mount [C]', - 'Commit root', - - // In a subsequent commit, render a placeholder - 'Loading...', 'Mount [Loading...]', + 'Commit root', ]); expect(ReactNoop.getChildrenAsJSX()).toEqual( diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 18b42dde793c5..a811cec410358 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -2501,7 +2501,7 @@ describe('Profiler', () => { expect(renderer.toJSON()).toEqual(['loading', 'initial']); expect(onInteractionScheduledWorkCompleted).not.toHaveBeenCalled(); - expect(onRender).toHaveBeenCalledTimes(2); // Sync null commit, placeholder commit + expect(onRender).toHaveBeenCalledTimes(1); expect(onRender.mock.calls[0][6]).toMatchInteractions([ initialRenderInteraction, ]); @@ -2535,13 +2535,10 @@ describe('Profiler', () => { }); expect(renderer.toJSON()).toEqual(['loading', 'updated']); - expect(onRender).toHaveBeenCalledTimes(2); // Sync null commit, placeholder commit + expect(onRender).toHaveBeenCalledTimes(1); expect(onRender.mock.calls[0][6]).toMatchInteractions([ highPriUpdateInteraction, ]); - expect(onRender.mock.calls[1][6]).toMatchInteractions([ - highPriUpdateInteraction, - ]); onRender.mockClear(); expect(onInteractionScheduledWorkCompleted).not.toHaveBeenCalled();