From 9d0122c1c24bd9d7d41d29692ef72062814a63fe Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 9 Oct 2018 15:53:31 -0700 Subject: [PATCH 01/13] Store the start time on `updateQueue` instead of `stateNode` Originally I did this to free the `stateNode` field to store a second set of children. I don't we'll need this anymore, since we use fragment fibers instead. But I still think using `updateQueue` makes more sense so I'll leave this in. --- .../src/ReactFiberBeginWork.js | 30 +++++++++---------- .../src/ReactFiberCommitWork.js | 19 ++++++------ .../src/ReactFiberUnwindWork.js | 11 ++++--- 3 files changed, 29 insertions(+), 31 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 8a26c84b3928d..aaa0755c95e53 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -65,7 +65,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, @@ -951,24 +951,24 @@ function updateSuspenseComponent( const alreadyCaptured = (workInProgress.effectTag & DidCapture) === NoEffect; let nextDidTimeout; - if (current !== null && workInProgress.updateQueue !== null) { - // We're outside strict mode. Something inside this Placeholder boundary - // suspended during the last commit. Switch to the placholder. - workInProgress.updateQueue = null; - nextDidTimeout = true; + if ((workInProgress.mode & StrictMode) === NoContext) { + // We're outside strict mode. + if (workInProgress.updateQueue !== null) { + // Something inside this boundary suspended during the last commit. Switch + // to the placholder. + workInProgress.updateQueue = null; + nextDidTimeout = true; + } else { + nextDidTimeout = false; + } } else { - nextDidTimeout = !alreadyCaptured; - } - - if ((workInProgress.mode & StrictMode) !== NoEffect) { - if (nextDidTimeout) { + if (alreadyCaptured) { + nextDidTimeout = false; + } else { // If the timed-out view commits, schedule an update effect to record // the committed time. + nextDidTimeout = true; workInProgress.effectTag |= Update; - } else { - // The state node points to the time at which placeholder timed out. - // We can clear it once we switch back to the normal children. - workInProgress.stateNode = null; } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 52ab8d5a47593..a44b0a21e31f6 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -39,7 +39,6 @@ import { clearCaughtError, } from 'shared/ReactErrorUtils'; import { - NoEffect, ContentReset, Placement, Snapshot, @@ -78,7 +77,7 @@ import { requestCurrentTime, scheduleWork, } from './ReactFiberScheduler'; -import {StrictMode} from './ReactTypeOfMode'; +import {NoContext, StrictMode} from './ReactTypeOfMode'; const emptyObject = {}; @@ -352,19 +351,19 @@ function commitLifeCycles( return; } case SuspenseComponent: { - if ((finishedWork.mode & StrictMode) === NoEffect) { - // In loose mode, a placeholder times out by scheduling a synchronous - // update in the commit phase. Use `updateQueue` field to signal that - // the Timeout needs to switch to the placeholder. We don't need an - // entire queue. Any non-null value works. + if ((finishedWork.mode & StrictMode) === NoContext) { + // In loose mode, a suspense boundary times out by scheduling a + // synchronous update in the commit phase. Use `updateQueue` field to + // signal that the Timeout needs to switch to the placeholder. We don't + // need an entire queue. Any non-null value works. // $FlowFixMe - Intentionally using a value other than an UpdateQueue. finishedWork.updateQueue = emptyObject; scheduleWork(finishedWork, Sync); } else { // In strict mode, the Update effect is used to record the time at - // which the placeholder timed out. - const currentTime = requestCurrentTime(); - finishedWork.stateNode = {timedOutAt: currentTime}; + // which the children timed out. + // $FlowFixMe - Intentionally using a value other than an UpdateQueue. + finishedWork.updateQueue = {timedOutAt: requestCurrentTime()}; } return; } diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 7918f64e6c696..91fcf72b228f7 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -176,17 +176,16 @@ function throwException( do { if (workInProgress.tag === SuspenseComponent) { const current = workInProgress.alternate; - if ( - current !== null && - current.memoizedState === true && - current.stateNode !== null - ) { + if (current !== null && current.updateQueue !== null) { // Reached a placeholder that already timed out. Each timed out // placeholder acts as the root of a new suspense boundary. // Use the time at which the placeholder timed out as the start time // for the current render. - const timedOutAt = current.stateNode.timedOutAt; + const suspenseInfo: {| + timedOutAt: ExpirationTime, + |} = (current.updateQueue: any); + const timedOutAt = suspenseInfo.timedOutAt; startTimeMs = expirationTimeToMs(timedOutAt); // Do not search any further. From af0b2198ab1f0c8bb433e93ebd2842c438375079 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 10 Oct 2018 17:44:15 -0700 Subject: [PATCH 02/13] Use fragment fibers to keep the primary and fallback children separate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the children timeout, we switch to showing the fallback children in place of the "primary" children. However, we don't want to delete the primary children because then their state will be lost (both the React state and the host state, e.g. uncontrolled form inputs). Instead we keep them mounted and hide them. Both the fallback children AND the primary children are rendered at the same time. Once the primary children are un-suspended, we can delete the fallback children — don't need to preserve their state. The two sets of children are siblings in the host environment, but semantically, for purposes of reconciliation, they are two separate sets. So we store them using two fragment fibers. However, we want to avoid allocating extra fibers for every placeholder. They're only necessary when the children time out, because that's the only time when both sets are mounted. So, the extra fragment fibers are only used if the children time out. Otherwise, we render the primary children directly. This requires some custom reconciliation logic to preserve the state of the primary children. It's essentially a very basic form of re-parenting. --- .../src/ReactFiberBeginWork.js | 220 +++++++++++++++--- .../src/ReactFiberCompleteWork.js | 11 +- .../src/ReactFiberScheduler.js | 24 +- .../src/ReactFiberUnwindWork.js | 7 +- ...tSuspenseWithNoopRenderer-test.internal.js | 51 ++-- 5 files changed, 228 insertions(+), 85 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index aaa0755c95e53..204ead5b54440 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -103,7 +103,11 @@ import { } from './ReactFiberClassComponent'; import {readLazyComponentType} from './ReactFiberLazyComponent'; import {getResultFromResolvedThenable} from 'shared/ReactLazyComponent'; -import {resolveLazyComponentTag} from './ReactFiber'; +import { + resolveLazyComponentTag, + createFiberFromFragment, + createWorkInProgress, +} from './ReactFiber'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; @@ -701,19 +705,13 @@ function resolveDefaultProps(Component, baseProps) { return baseProps; } -function mountIndeterminateComponent( +function updateIndeterminateComponent( current, workInProgress, Component, updateExpirationTime, renderExpirationTime, ) { - invariant( - current === null, - 'An indeterminate component should never have mounted. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); - const props = workInProgress.pendingProps; if ( typeof Component === 'object' && @@ -944,6 +942,7 @@ function updateSuspenseComponent( workInProgress, renderExpirationTime, ) { + const mode = workInProgress.mode; const nextProps = workInProgress.pendingProps; // Check if we already attempted to render the normal state. If we did, @@ -951,11 +950,11 @@ function updateSuspenseComponent( const alreadyCaptured = (workInProgress.effectTag & DidCapture) === NoEffect; let nextDidTimeout; - if ((workInProgress.mode & StrictMode) === NoContext) { + if ((mode & StrictMode) === NoContext) { // We're outside strict mode. if (workInProgress.updateQueue !== null) { - // Something inside this boundary suspended during the last commit. Switch - // to the placholder. + // Something inside this Placeholder boundary suspended during the last + // commit. Switch to the placholder. workInProgress.updateQueue = null; nextDidTimeout = true; } else { @@ -973,7 +972,7 @@ function updateSuspenseComponent( } // If the `children` prop is a function, treat it like a render prop. - // TODO: This is temporary until we finalize a lower level API. + // TODO: Remove this, or put it behind a feature flag const children = nextProps.children; let nextChildren; if (typeof children === 'function') { @@ -982,29 +981,169 @@ function updateSuspenseComponent( nextChildren = nextDidTimeout ? nextProps.fallback : children; } - if (current !== null && nextDidTimeout !== workInProgress.memoizedState) { - // We're about to switch from the placeholder children to the normal - // children, or vice versa. These are two different conceptual sets that - // happen to be stored in the same set. Call this special function to - // force the new set not to match with the current set. - // TODO: The proper way to model this is by storing each set separately. - forceUnmountCurrentAndReconcile( - current, - workInProgress, - nextChildren, - renderExpirationTime, - ); + // This next part is a bit confusing. If the children timeout, we switch to + // showing the fallback children in place of the "primary" children. + // However, we don't want to delete the primary children because then their + // state will be lost (both the React state and the host state, e.g. + // uncontrolled form inputs). Instead we keep them mounted and hide them. + // Both the fallback children AND the primary children are rendered at the + // same time. Once the primary children are un-suspended, we can delete + // the fallback children — don't need to preserve their state. + // + // The two sets of children are siblings in the host environment, but + // semantically, for purposes of reconciliation, they are two separate sets. + // So we store them using two fragment fibers. + // + // However, we want to avoid allocating extra fibers for every placeholder. + // They're only necessary when the children time out, because that's the + // only time when both sets are mounted. + // + // So, the extra fragment fibers are only used if the children time out. + // Otherwise, we render the primary children directly. This requires some + // custom reconciliation logic to preserve the state of the primary + // children. It's essentially a very basic form of re-parenting. + + // `child` points to the child fiber. In the normal case, this is the first + // fiber of the primary children set. In the timed-out case, it's a + // a fragment fiber containing the primary children. + let child; + // `next` points to the next fiber React should render. In the normal case, + // it's the same as `child`: the first fiber of the primary children set. + // In the timed-out case, it's a fragment fiber containing the *fallback* + // children -- we skip over the primary children entirely. + let next; + if (current === null) { + // This is the initial mount. This branch is pretty simple because there's + // no previous state that needs to be preserved. + if (nextDidTimeout) { + // Mount separate fragments for primary and fallback children. + const primaryChildFragment = createFiberFromFragment( + null, + mode, + NoWork, + null, + ); + const fallbackChildFragment = createFiberFromFragment( + nextChildren, + mode, + renderExpirationTime, + null, + ); + primaryChildFragment.sibling = fallbackChildFragment; + child = primaryChildFragment; + // Skip the primary children, and continue working on the + // fallback children. + next = fallbackChildFragment; + child.return = next.return = workInProgress; + } else { + // Mount the primary children without an intermediate fragment fiber. + child = next = mountChildFibers( + workInProgress, + null, + nextChildren, + renderExpirationTime, + ); + } } else { - reconcileChildren( - current, - workInProgress, - nextChildren, - renderExpirationTime, - ); + // This is an update. This branch is more complicated because we need to + // ensure the state of the primary children is preserved. + const prevDidTimeout = current.memoizedState === true; + if (prevDidTimeout) { + // The current tree already timed out. That means each child set is + // wrapped in a fragment fiber. + const currentPrimaryChildFragment: Fiber = (current.child: any); + const currentFallbackChildFragment: Fiber = (currentPrimaryChildFragment.sibling: any); + if (nextDidTimeout) { + // Still timed out. Reuse the current primary children by cloning + // its fragment. We're going to skip over these entirely. + const primaryChildFragment = createWorkInProgress( + currentFallbackChildFragment, + currentFallbackChildFragment.pendingProps, + NoWork, + ); + primaryChildFragment.effectTag |= Placement; + // Clone the fallback child fragment, too. These we'll continue + // working on. + const fallbackChildFragment = (primaryChildFragment.sibling = createWorkInProgress( + currentFallbackChildFragment, + nextChildren, + currentFallbackChildFragment.expirationTime, + )); + fallbackChildFragment.effectTag |= Placement; + child = primaryChildFragment; + // Skip the primary children, and continue working on the + // fallback children. + next = fallbackChildFragment; + child.return = next.return = workInProgress; + } else { + // No longer suspended. Switch back to showing the primary children, + // and remove the intermediate fragment fiber. + const currentPrimaryChild = currentPrimaryChildFragment.child; + const currentFallbackChild = currentFallbackChildFragment.child; + const primaryChild = reconcileChildFibers( + workInProgress, + currentPrimaryChild, + nextChildren, + renderExpirationTime, + ); + // Delete the fallback children. + reconcileChildFibers( + workInProgress, + currentFallbackChild, + null, + renderExpirationTime, + ); + // Continue rendering the children, like we normally do. + child = next = primaryChild; + } + } else { + // The current tree has not already timed out. That means the primary + // children are not wrapped in a fragment fiber. + const currentPrimaryChild: Fiber = (current.child: any); + if (nextDidTimeout) { + // Timed out. Wrap the children in a fragment fiber to keep them + // separate from the fallback children. + const primaryChildFragment = createFiberFromFragment( + // It shouldn't matter what the pending props are because we aren't + // going to render this fragment. + null, + mode, + NoWork, + null, + ); + primaryChildFragment.effectTag |= Placement; + primaryChildFragment.child = currentPrimaryChild; + currentPrimaryChild.return = primaryChildFragment; + // Create a fragment from the fallback children, too. + const fallbackChildFragment = (primaryChildFragment.sibling = createFiberFromFragment( + nextChildren, + mode, + renderExpirationTime, + null, + )); + fallbackChildFragment.effectTag |= Placement; + child = primaryChildFragment; + // Skip the primary children, and continue working on the + // fallback children. + next = fallbackChildFragment; + child.return = next.return = workInProgress; + } else { + // Still haven't timed out. Continue rendering the children, like we + // normally do. + next = child = reconcileChildFibers( + workInProgress, + currentPrimaryChild, + nextChildren, + renderExpirationTime, + ); + } + } } + workInProgress.memoizedProps = nextProps; workInProgress.memoizedState = nextDidTimeout; - return workInProgress.child; + workInProgress.child = child; + return next; } function updatePortalComponent( @@ -1290,6 +1429,23 @@ function beginWork( workInProgress.effectTag |= Update; } break; + case SuspenseComponent: { + const nextDidTimeout = workInProgress.memoizedState; + const child = bailoutOnAlreadyFinishedWork( + current, + workInProgress, + renderExpirationTime, + ); + if (child !== null) { + if (nextDidTimeout) { + return child.sibling; + } else { + return child; + } + } else { + return null; + } + } } return bailoutOnAlreadyFinishedWork( current, @@ -1305,7 +1461,7 @@ function beginWork( switch (workInProgress.tag) { case IndeterminateComponent: { const Component = workInProgress.type; - return mountIndeterminateComponent( + return updateIndeterminateComponent( current, workInProgress, Component, diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index c0b53764a5694..d8eb6a810749f 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -353,6 +353,8 @@ function completeWork( const newProps = workInProgress.pendingProps; switch (workInProgress.tag) { + case IndeterminateComponent: + break; case FunctionComponent: case FunctionComponentLazy: break; @@ -529,15 +531,6 @@ function completeWork( case PureComponent: case PureComponentLazy: break; - // Error cases - case IndeterminateComponent: - invariant( - false, - 'An indeterminate component should have become determinate before ' + - 'completing. This error is likely caused by a bug in React. Please ' + - 'file an issue.', - ); - // eslint-disable-next-line no-fallthrough default: invariant( false, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 5b0353679b95e..be759892b6c00 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -110,7 +110,12 @@ import { computeAsyncExpiration, computeInteractiveExpiration, } from './ReactFiberExpirationTime'; -import {ConcurrentMode, ProfileMode, NoContext} from './ReactTypeOfMode'; +import { + ConcurrentMode, + ProfileMode, + NoContext, + StrictMode, +} from './ReactTypeOfMode'; import {enqueueUpdate, resetCurrentlyProcessingQueue} from './ReactUpdateQueue'; import {createCapturedValue} from './ReactCapturedValue'; import { @@ -1563,7 +1568,8 @@ function renderDidError() { function retrySuspendedRoot( root: FiberRoot, - fiber: Fiber, + boundaryFiber: Fiber, + sourceFiber: Fiber, suspendedTime: ExpirationTime, ) { let retryTime; @@ -1576,18 +1582,18 @@ function retrySuspendedRoot( } else { // Suspense already timed out. Compute a new expiration time const currentTime = requestCurrentTime(); - retryTime = computeExpirationForFiber(currentTime, fiber); + retryTime = computeExpirationForFiber(currentTime, boundaryFiber); markPendingPriorityLevel(root, retryTime); } - // TODO: If the placeholder fiber has already rendered the primary children + // TODO: If the suspense fiber has already rendered the primary children // without suspending (that is, all of the promises have already resolved), // we should not trigger another update here. One case this happens is when // we are in sync mode and a single promise is thrown both on initial render // and on update; we attach two .then(retrySuspendedRoot) callbacks and each // one performs Sync work, rerendering the Suspense. - if ((fiber.mode & ConcurrentMode) !== NoContext) { + if ((boundaryFiber.mode & ConcurrentMode) !== NoContext) { if (root === nextRoot && nextRenderExpirationTime === suspendedTime) { // Received a ping at the same priority level at which we're currently // rendering. Restart from the root. @@ -1595,7 +1601,13 @@ function retrySuspendedRoot( } } - scheduleWorkToRoot(fiber, retryTime); + scheduleWorkToRoot(boundaryFiber, retryTime); + if ((boundaryFiber.mode & StrictMode) === NoContext) { + // Outside of strict mode, we must schedule an update on the source fiber, + // too, since it already committed. + scheduleWorkToRoot(sourceFiber, retryTime); + } + const rootExpirationTime = root.expirationTime; if (rootExpirationTime !== NoWork) { requestWork(root, rootExpirationTime); diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 91fcf72b228f7..3ee7b99a366bd 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -18,7 +18,6 @@ import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing'; import getComponentName from 'shared/getComponentName'; import warningWithoutStack from 'shared/warningWithoutStack'; import { - IndeterminateComponent, FunctionComponent, ClassComponent, ClassComponentLazy, @@ -226,6 +225,7 @@ function throwException( null, root, workInProgress, + sourceFiber, pingTime, ); if (enableSchedulerTracing) { @@ -253,11 +253,6 @@ function throwException( renderExpirationTime, ); sourceFiber.effectTag &= ~Incomplete; - if (sourceFiber.tag === IndeterminateComponent) { - // Let's just assume it's a function component. This fiber will - // be unmounted in the immediate next commit, anyway. - sourceFiber.tag = FunctionComponent; - } if ( sourceFiber.tag === ClassComponent || diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index dd224fb5c6d29..14ce6cc9beaa4 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -966,7 +966,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Loading (3)', 'Promise resolved [Step: 1]', 'Step: 1', - 'Sibling', ]); expect(ReactNoop.getChildren()).toEqual([ span('Step: 1'), @@ -988,6 +987,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Loading (3)', ]); expect(ReactNoop.getChildren()).toEqual([ + span('Sibling'), span('Loading (1)'), span('Loading (2)'), span('Loading (3)'), @@ -996,13 +996,10 @@ describe('ReactSuspenseWithNoopRenderer', () => { await advanceTimers(100); expect(ReactNoop.flush()).toEqual([ 'Promise resolved [Step: 2]', - // TODO: The state of the children is lost when switching back. Revisit - // this in the follow up PR. - 'Step: 1', - 'Sibling', + 'Step: 2', ]); expect(ReactNoop.getChildren()).toEqual([ - span('Step: 1'), + span('Step: 2'), span('Sibling'), ]); }); @@ -1203,9 +1200,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { // The placeholder is rendered in a subsequent commit 'Loading...', 'Promise resolved [Async: 1]', - 'Before', 'Async: 1', - 'After', ]); expect(ReactNoop.getChildren()).toEqual([ span('Before'), @@ -1245,6 +1240,8 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Loading...', ]); expect(ReactNoop.getChildren()).toEqual([ + span('Before'), + span('After'), span('Loading...'), span('Before'), @@ -1257,16 +1254,12 @@ describe('ReactSuspenseWithNoopRenderer', () => { await advanceTimers(100); expect(ReactNoop.clearYields()).toEqual([ 'Promise resolved [Async: 2]', - 'Before', - 'Async: 1', - 'After', + 'Async: 2', ]); expect(ReactNoop.getChildren()).toEqual([ span('Before'), - // TODO: The state of the children is lost when switching back. Revisit - // this in the follow up PR. - span('Async: 1'), + span('Async: 2'), span('After'), span('Before'), @@ -1332,19 +1325,16 @@ describe('ReactSuspenseWithNoopRenderer', () => { // placeholder. This should be a mount, not an update. 'Mount [Loading...]', ]); - expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + expect(ReactNoop.getChildren()).toEqual([ + span('A'), + span('C'), - await advanceTimers(1000); - expect(ReactNoop.expire(1000)).toEqual([ - 'Promise resolved [B]', - 'A', - 'B', - 'C', - 'Mount [A]', - 'Mount [B]', - 'Mount [C]', + span('Loading...'), ]); + await advanceTimers(1000); + expect(ReactNoop.expire(1000)).toEqual(['Promise resolved [B]', 'B']); + expect(ReactNoop.getChildren()).toEqual([ span('A'), span('B'), @@ -1776,16 +1766,13 @@ describe('ReactSuspenseWithNoopRenderer', () => { // In a subsequent commit, render a placeholder 'Loading...', - - // A, B, and C are unmounted, but we skip calling B's componentWillUnmount - 'Unmount [A]', - 'Unmount [C]', - - // Force delete all the existing children when switching to the - // placeholder. This should be a mount, not an update. 'Mount [Loading...]', ]); - expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + expect(ReactNoop.getChildren()).toEqual([ + span('A'), + span('C'), + span('Loading...'), + ]); }); }); From dd9c5c5831c425d6634912f0c3ef3e17e8e10f69 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 11 Oct 2018 15:12:53 -0700 Subject: [PATCH 03/13] Use `memoizedState` to store various pieces of SuspenseComponent's state SuspenseComponent has three pieces of state: - alreadyCaptured: Whether a component in the child subtree already suspended. If true, subsequent suspends should bubble up to the next boundary. - didTimeout: 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. - timedOutAt: The time at which the boundary timed out. This is separate from `didTimeout` because it's not set unless the boundary actually commits. These were previously spread across several fields. This happens to make the non-strict case a bit less hacky; the logic for that special case is now mostly localized to the UnwindWork module. --- .../src/ReactFiberBeginWork.js | 55 ++++++++++++------- .../src/ReactFiberCommitWork.js | 35 +++++++----- .../src/ReactFiberSuspenseComponent.js | 25 +++++++++ .../src/ReactFiberUnwindWork.js | 55 +++++++++++++------ 4 files changed, 118 insertions(+), 52 deletions(-) create mode 100644 packages/react-reconciler/src/ReactFiberSuspenseComponent.js diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 204ead5b54440..ff76a13db832e 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -11,6 +11,8 @@ import type {ReactProviderType, ReactContext} from 'shared/ReactTypes'; import type {Fiber} from 'react-reconciler/src/ReactFiber'; import type {FiberRoot} from './ReactFiberRoot'; import type {ExpirationTime} from './ReactFiberExpirationTime'; +import type {SuspenseState} from './ReactFiberSuspenseComponent'; + import checkPropTypes from 'prop-types/checkPropTypes'; import { @@ -65,7 +67,7 @@ import { } from './ReactChildFiber'; import {processUpdateQueue} from './ReactUpdateQueue'; import {NoWork, Never} from './ReactFiberExpirationTime'; -import {ConcurrentMode, StrictMode, NoContext} from './ReactTypeOfMode'; +import {ConcurrentMode, StrictMode} from './ReactTypeOfMode'; import { shouldSetTextContent, shouldDeprioritizeSubtree, @@ -945,28 +947,37 @@ function updateSuspenseComponent( const mode = workInProgress.mode; const nextProps = workInProgress.pendingProps; - // Check if we already attempted to render the normal state. If we did, - // and we timed out, render the placeholder state. - const alreadyCaptured = (workInProgress.effectTag & DidCapture) === NoEffect; - + // 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; let nextDidTimeout; - if ((mode & StrictMode) === NoContext) { - // We're outside strict mode. - if (workInProgress.updateQueue !== null) { - // Something inside this Placeholder boundary suspended during the last - // commit. Switch to the placholder. - workInProgress.updateQueue = null; - nextDidTimeout = true; - } else { - nextDidTimeout = false; - } + if (nextState === null) { + // An empty suspense state means this boundary has not yet timed out. + nextDidTimeout = false; } else { - if (alreadyCaptured) { + if (!nextState.alreadyCaptured) { + // Since we haven't already suspended during this commit, clear the + // existing suspense state. We'll try rendering again. nextDidTimeout = false; + nextState = null; } else { - // If the timed-out view commits, schedule an update effect to record - // the committed time. + // Something in this boundary's subtree already suspended. Switch to + // rendering the fallback children. Set `alreadyCaptured` to true. nextDidTimeout = 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; + } + // If this render commits, schedule an update effect to record the timed- + // out time. workInProgress.effectTag |= Update; } } @@ -1047,7 +1058,8 @@ function updateSuspenseComponent( } else { // This is an update. This branch is more complicated because we need to // ensure the state of the primary children is preserved. - const prevDidTimeout = current.memoizedState === true; + const prevState = current.memoizedState; + const prevDidTimeout = prevState !== null && prevState.didTimeout; if (prevDidTimeout) { // The current tree already timed out. That means each child set is // wrapped in a fragment fiber. @@ -1141,7 +1153,7 @@ function updateSuspenseComponent( } workInProgress.memoizedProps = nextProps; - workInProgress.memoizedState = nextDidTimeout; + workInProgress.memoizedState = nextState; workInProgress.child = child; return next; } @@ -1430,13 +1442,14 @@ function beginWork( } break; case SuspenseComponent: { - const nextDidTimeout = workInProgress.memoizedState; const child = bailoutOnAlreadyFinishedWork( current, workInProgress, renderExpirationTime, ); if (child !== null) { + const nextState = workInProgress.memoizedState; + const nextDidTimeout = nextState !== null && nextState.didTimeout; if (nextDidTimeout) { return child.sibling; } else { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index a44b0a21e31f6..6648be86b4ead 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -18,6 +18,7 @@ import type {Fiber} from './ReactFiber'; import type {FiberRoot} from './ReactFiberRoot'; import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {CapturedValue, CapturedError} from './ReactCapturedValue'; +import type {SuspenseState} from './ReactFiberSuspenseComponent'; import { enableSchedulerTracing, @@ -48,7 +49,7 @@ import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; import warningWithoutStack from 'shared/warningWithoutStack'; -import {Sync} from './ReactFiberExpirationTime'; +import {NoWork, Sync} from './ReactFiberExpirationTime'; import {onCommitUnmount} from './ReactFiberDevToolsHook'; import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf'; import {getStackByFiberInDevAndProd} from './ReactCurrentFiber'; @@ -77,9 +78,6 @@ import { requestCurrentTime, scheduleWork, } from './ReactFiberScheduler'; -import {NoContext, StrictMode} from './ReactTypeOfMode'; - -const emptyObject = {}; let didWarnAboutUndefinedSnapshotBeforeUpdate: Set | null = null; if (__DEV__) { @@ -351,19 +349,26 @@ function commitLifeCycles( return; } case SuspenseComponent: { - if ((finishedWork.mode & StrictMode) === NoContext) { - // In loose mode, a suspense boundary times out by scheduling a - // synchronous update in the commit phase. Use `updateQueue` field to - // signal that the Timeout needs to switch to the placeholder. We don't - // need an entire queue. Any non-null value works. - // $FlowFixMe - Intentionally using a value other than an UpdateQueue. - finishedWork.updateQueue = emptyObject; + let newState: SuspenseState | null = finishedWork.memoizedState; + if (newState === null) { + // 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. + newState = finishedWork.memoizedState = { + alreadyCaptured: true, + didTimeout: false, + timedOutAt: NoWork, + }; scheduleWork(finishedWork, Sync); } else { - // In strict mode, the Update effect is used to record the time at - // which the children timed out. - // $FlowFixMe - Intentionally using a value other than an UpdateQueue. - finishedWork.updateQueue = {timedOutAt: requestCurrentTime()}; + 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(); + } } return; } diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js new file mode 100644 index 0000000000000..285f7893f4734 --- /dev/null +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -0,0 +1,25 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +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, +|}; diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 3ee7b99a366bd..861ac82591af5 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -60,7 +60,7 @@ import { isAlreadyFailedLegacyErrorBoundary, retrySuspendedRoot, } from './ReactFiberScheduler'; -import {Sync} from './ReactFiberExpirationTime'; +import {NoWork, Sync} from './ReactFiberExpirationTime'; import invariant from 'shared/invariant'; import maxSigned31BitInt from './maxSigned31BitInt'; @@ -175,20 +175,16 @@ function throwException( do { if (workInProgress.tag === SuspenseComponent) { const current = workInProgress.alternate; - if (current !== null && current.updateQueue !== null) { - // Reached a placeholder that already timed out. Each timed out - // placeholder acts as the root of a new suspense boundary. - - // Use the time at which the placeholder timed out as the start time - // for the current render. - const suspenseInfo: {| - timedOutAt: ExpirationTime, - |} = (current.updateQueue: any); - const timedOutAt = suspenseInfo.timedOutAt; - startTimeMs = expirationTimeToMs(timedOutAt); - - // Do not search any further. - break; + if (current !== null) { + const currentState = current.memoizedState; + if (currentState !== null && currentState.didTimeout) { + // Reached a boundary that already timed out. Do not search + // any further. + const timedOutAt = currentState.timedOutAt; + startTimeMs = expirationTimeToMs(timedOutAt); + // Do not search any further. + break; + } } let timeoutPropMs = workInProgress.pendingProps.maxDuration; if (typeof timeoutPropMs === 'number') { @@ -209,7 +205,8 @@ function throwException( workInProgress = returnFiber; do { if (workInProgress.tag === SuspenseComponent) { - const didTimeout = workInProgress.memoizedState; + const state = workInProgress.memoizedState; + const didTimeout = state !== null && state.didTimeout; if (!didTimeout) { // Found the nearest boundary. @@ -431,6 +428,32 @@ 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 = current !== null ? current.memoizedState : null; + let nextState = workInProgress.memoizedState; + if (currentState === 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: currentState.didTimeout, + timedOutAt: currentState.timedOutAt, + }; + } else { + // Already have a clone, so it's safe to mutate. + nextState.alreadyCaptured = true; + } + workInProgress.memoizedState = nextState; + // Re-render the boundary. return workInProgress; } return null; From 1c0091d8b465994408d787a0c9616858169bb18f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 11 Oct 2018 19:08:40 -0700 Subject: [PATCH 04/13] Include `hidden` prop in noop renderer's output This will be used in subsequent commits to test that timed-out children are properly hidden. Also adds getChildrenAsJSX() method as an alternative to using getChildren(). (Ideally all our tests would use test renderer #oneday.) --- .../src/createReactNoop.js | 176 ++++++++++++---- .../ReactExpiration-test.internal.js | 2 +- .../src/__tests__/ReactFragment-test.js | 16 +- ...tIncrementalErrorHandling-test.internal.js | 4 +- ...eactIncrementalScheduling-test.internal.js | 2 +- ...actIncrementalSideEffects-test.internal.js | 189 ++++++++++++------ .../ReactIncrementalTriangle-test.internal.js | 2 +- .../ReactIncrementalUpdates-test.internal.js | 2 +- .../ReactNewContext-test.internal.js | 7 +- .../src/__tests__/ReactPersistent-test.js | 8 +- .../src/__tests__/ReactPure-test.internal.js | 2 +- ...tSuspenseWithNoopRenderer-test.internal.js | 23 ++- .../src/__tests__/ReactTopLevelText-test.js | 4 +- .../__tests__/ReactProfiler-test.internal.js | 12 +- .../react/src/__tests__/forwardRef-test.js | 10 +- 15 files changed, 316 insertions(+), 143 deletions(-) diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 00fab26d8e2b7..360e8d732b6a0 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -20,16 +20,22 @@ import type {ReactNodeList} from 'shared/ReactTypes'; import * as ReactPortal from 'shared/ReactPortal'; import expect from 'expect'; +import {REACT_FRAGMENT_TYPE, REACT_ELEMENT_TYPE} from 'shared/ReactSymbols'; -type Container = {rootID: string, children: Array}; -type Props = {prop: any, hidden?: boolean, children?: mixed}; +type Container = { + rootID: string, + children: Array, +}; +type Props = {prop: any, hidden: boolean, children?: mixed}; type Instance = {| type: string, id: number, children: Array, + text: string | null, prop: any, + hidden: boolean, |}; -type TextInstance = {|text: string, id: number|}; +type TextInstance = {|text: string, id: number, hidden: boolean|}; const NO_CONTEXT = {}; const UPDATE_SIGNAL = {}; @@ -164,6 +170,47 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { removeChildFromContainerOrInstance(parentInstance, child); } + function cloneInstance( + instance: Instance, + updatePayload: null | Object, + type: string, + oldProps: Props, + newProps: Props, + internalInstanceHandle: Object, + keepChildren: boolean, + recyclableInstance: null | Instance, + ): Instance { + const clone = { + id: instance.id, + type: type, + children: keepChildren ? instance.children : [], + text: shouldSetTextContent(type, newProps) + ? (newProps.children: any) + '' + : null, + prop: newProps.prop, + hidden: newProps.hidden === true, + }; + Object.defineProperty(clone, 'id', { + value: clone.id, + enumerable: false, + }); + Object.defineProperty(clone, 'text', { + value: clone.text, + enumerable: false, + }); + hostCloneCounter++; + return clone; + } + + function shouldSetTextContent(type: string, props: Props): boolean { + if (type === 'errorInBeginPhase') { + throw new Error('Error in host config.'); + } + return ( + typeof props.children === 'string' || typeof props.children === 'number' + ); + } + let elapsedTimeInMs = 0; const sharedHostConfig = { @@ -187,10 +234,18 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { id: instanceCounter++, type: type, children: [], + text: shouldSetTextContent(type, props) + ? (props.children: any) + '' + : null, prop: props.prop, + hidden: props.hidden === true, }; // Hide from unit tests Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false}); + Object.defineProperty(inst, 'text', { + value: inst.text, + enumerable: false, + }); return inst; }, @@ -228,14 +283,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return UPDATE_SIGNAL; }, - shouldSetTextContent(type: string, props: Props): boolean { - if (type === 'errorInBeginPhase') { - throw new Error('Error in host config.'); - } - return ( - typeof props.children === 'string' || typeof props.children === 'number' - ); - }, + shouldSetTextContent, shouldDeprioritizeSubtree(type: string, props: Props): boolean { return !!props.hidden; @@ -247,7 +295,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { hostContext: Object, internalInstanceHandle: Object, ): TextInstance { - const inst = {text: text, id: instanceCounter++}; + const inst = {text: text, id: instanceCounter++, hidden: false}; // Hide from unit tests Object.defineProperty(inst, 'id', {value: inst.id, enumerable: false}); return inst; @@ -324,6 +372,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { } hostUpdateCounter++; instance.prop = newProps.prop; + instance.hidden = newProps.hidden === true; + if (shouldSetTextContent(type, newProps)) { + instance.text = (newProps.children: any) + ''; + } }, commitTextUpdate( @@ -342,36 +394,16 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { removeChild, removeChildFromContainer, - resetTextContent(instance: Instance): void {}, + resetTextContent(instance: Instance): void { + instance.text = null; + }, } : { ...sharedHostConfig, supportsMutation: false, supportsPersistence: true, - cloneInstance( - instance: Instance, - updatePayload: null | Object, - type: string, - oldProps: Props, - newProps: Props, - internalInstanceHandle: Object, - keepChildren: boolean, - recyclableInstance: null | Instance, - ): Instance { - const clone = { - id: instance.id, - type: type, - children: keepChildren ? instance.children : [], - prop: newProps.prop, - }; - Object.defineProperty(clone, 'id', { - value: clone.id, - enumerable: false, - }); - hostCloneCounter++; - return clone; - }, + cloneInstance, createContainerChildSet( container: Container, @@ -439,6 +471,59 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { } } + function childToJSX(child, text) { + if (text !== null) { + return text; + } + if (child === null) { + return null; + } + if (typeof child === 'string') { + return child; + } + if (Array.isArray(child)) { + if (child.length === 0) { + return null; + } + if (child.length === 1) { + return childToJSX(child[0], null); + } + // $FlowFixMe + const children = child.map(c => childToJSX(c, null)); + if (children.every(c => typeof c === 'string' || typeof c === 'number')) { + return children.join(''); + } + return children; + } + if (Array.isArray(child.children)) { + // This is an instance. + const instance: Instance = (child: any); + const children = childToJSX(instance.children, instance.text); + const props = ({prop: instance.prop}: any); + if (instance.hidden) { + props.hidden = true; + } + if (children !== null) { + props.children = children; + } + return { + $$typeof: REACT_ELEMENT_TYPE, + type: instance.type, + key: null, + ref: null, + props: props, + _owner: null, + _store: __DEV__ ? {} : undefined, + }; + } + // This is a text instance + const textInstance: TextInstance = (child: any); + if (textInstance.hidden) { + return ''; + } + return textInstance.text; + } + const ReactNoop = { getChildren(rootID: string = DEFAULT_ROOT_ID) { const container = rootContainers.get(rootID); @@ -463,6 +548,25 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return root.current.stateNode.containerInfo; }, + getChildrenAsJSX(rootID: string = DEFAULT_ROOT_ID) { + const children = childToJSX(ReactNoop.getChildren(rootID), null); + if (children === null) { + return null; + } + if (Array.isArray(children)) { + return { + $$typeof: REACT_ELEMENT_TYPE, + type: REACT_FRAGMENT_TYPE, + key: null, + ref: null, + props: {children}, + _owner: null, + _store: __DEV__ ? {} : undefined, + }; + } + return children; + }, + createPortal( children: ReactNodeList, container: Container, diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js index 4de857e809f33..65f06b6dc3f51 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js @@ -23,7 +23,7 @@ describe('ReactExpiration', () => { }); function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } it('increases priority of updates as time progresses', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactFragment-test.js b/packages/react-reconciler/src/__tests__/ReactFragment-test.js index 9843d96e4a3bd..a742ac0884db8 100644 --- a/packages/react-reconciler/src/__tests__/ReactFragment-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFragment-test.js @@ -20,17 +20,19 @@ describe('ReactFragment', () => { ReactNoop = require('react-noop-renderer'); }); - function span(prop) { - return {type: 'span', children: [], prop}; + function div(...children) { + children = children.map( + c => (typeof c === 'string' ? {text: c, hidden: false} : c), + ); + return {type: 'div', children, prop: undefined, hidden: false}; } - function text(val) { - return {text: val}; + function span(prop) { + return {type: 'span', children: [], prop, hidden: false}; } - function div(...children) { - children = children.map(c => (typeof c === 'string' ? {text: c} : c)); - return {type: 'div', children, prop: undefined}; + function text(t) { + return {text: t, hidden: false}; } it('should render a single child via noop renderer', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 345c5b537f2c5..079a570202ea9 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -28,11 +28,11 @@ describe('ReactIncrementalErrorHandling', () => { function div(...children) { children = children.map(c => (typeof c === 'string' ? {text: c} : c)); - return {type: 'div', children, prop: undefined}; + return {type: 'div', children, prop: undefined, hidden: false}; } function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } function normalizeCodeLocInfo(str) { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js index 66fa25b24d348..b362585a5a5c7 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalScheduling-test.internal.js @@ -24,7 +24,7 @@ describe('ReactIncrementalScheduling', () => { }); function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } it('schedules and flushes deferred work', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js index 086b3f8f2a1e9..42f60af9aa5ea 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.internal.js @@ -24,16 +24,18 @@ describe('ReactIncrementalSideEffects', () => { }); function div(...children) { - children = children.map(c => (typeof c === 'string' ? text(c) : c)); - return {type: 'div', children, prop: undefined}; + children = children.map( + c => (typeof c === 'string' ? {text: c, hidden: false} : c), + ); + return {type: 'div', children, prop: undefined, hidden: false}; } function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } function text(t) { - return {text: t}; + return {text: t, hidden: false}; } it('can update child nodes of a host instance', () => { @@ -417,16 +419,33 @@ describe('ReactIncrementalSideEffects', () => { ReactNoop.render(); ReactNoop.flush(); - expect(ReactNoop.getChildren()).toEqual([div(div(span('foo')))]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( +
+ +
, + ); ReactNoop.render(); ReactNoop.flushDeferredPri(20); - expect(ReactNoop.getChildren()).toEqual([div(div(span('foo')))]); - + expect(ReactNoop.getChildrenAsJSX()).toEqual( +
+ +
, + ); ReactNoop.flush(); - expect(ReactNoop.getChildren()).toEqual([div(div(span('bar')))]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( +
+ +
, + ); }); it('can reuse side-effects after being preempted', () => { @@ -460,9 +479,14 @@ describe('ReactIncrementalSideEffects', () => { ReactNoop.render(); ReactNoop.flush(); - expect(ReactNoop.getChildren()).toEqual([ - div(div(span('Hi'), span('foo'))), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( + , + ); // Make a quick update which will schedule low priority work to // update the middle content. @@ -470,9 +494,14 @@ describe('ReactIncrementalSideEffects', () => { ReactNoop.flushDeferredPri(30); // The tree remains unchanged. - expect(ReactNoop.getChildren()).toEqual([ - div(div(span('Hi'), span('foo'))), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( + , + ); // The first Bar has already completed its update but we'll interrupt it to // render some higher priority work. The middle content will bailout so @@ -484,9 +513,14 @@ describe('ReactIncrementalSideEffects', () => { // we should be able to reuse the reconciliation work that we already did // without restarting. The side-effects should still be replayed. - expect(ReactNoop.getChildren()).toEqual([ - div(div(span('Hello'), span('World'))), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( + , + ); }); it('can reuse side-effects after being preempted, if shouldComponentUpdate is false', () => { @@ -525,9 +559,14 @@ describe('ReactIncrementalSideEffects', () => { ReactNoop.render(); ReactNoop.flush(); - expect(ReactNoop.getChildren()).toEqual([ - div(div(span('Hi'), span('foo'))), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( + , + ); // Make a quick update which will schedule low priority work to // update the middle content. @@ -535,9 +574,14 @@ describe('ReactIncrementalSideEffects', () => { ReactNoop.flushDeferredPri(35); // The tree remains unchanged. - expect(ReactNoop.getChildren()).toEqual([ - div(div(span('Hi'), span('foo'))), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( + , + ); // The first Bar has already completed its update but we'll interrupt it to // render some higher priority work. The middle content will bailout so @@ -549,9 +593,14 @@ describe('ReactIncrementalSideEffects', () => { // we should be able to reuse the reconciliation work that we already did // without restarting. The side-effects should still be replayed. - expect(ReactNoop.getChildren()).toEqual([ - div(div(span('Hello'), span('World'))), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( + , + ); }); it('can update a completed tree before it has a chance to commit', () => { @@ -597,7 +646,11 @@ describe('ReactIncrementalSideEffects', () => { ReactNoop.render(); ReactNoop.flush(); - expect(ReactNoop.getChildren()).toEqual([div(span(1))]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( + , + ); }); xit('can defer side-effects and resume them later on', () => { @@ -855,9 +908,16 @@ describe('ReactIncrementalSideEffects', () => { } ReactNoop.render(); ReactNoop.flush(); - expect(ReactNoop.getChildren()).toEqual([ - div(span(0), div(span(0), span(0), span(0))), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( +
+ + +
, + ); expect(ops).toEqual(['Foo', 'Bar', 'Bar', 'Bar']); @@ -865,18 +925,17 @@ describe('ReactIncrementalSideEffects', () => { ReactNoop.render(); ReactNoop.flushDeferredPri(70 + 5); - expect(ReactNoop.getChildren()).toEqual([ - div( - // Updated. - span(1), - div( - // Still not updated. - span(0), - span(0), - span(0), - ), - ), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( +
+ {/* Updated */} + + +
, + ); expect(ops).toEqual(['Foo', 'Bar', 'Bar']); ops = []; @@ -888,18 +947,18 @@ describe('ReactIncrementalSideEffects', () => { // TODO: The cycles it takes to do this could be lowered with further // optimizations. ReactNoop.flushDeferredPri(35); - expect(ReactNoop.getChildren()).toEqual([ - div( - // Updated. - span(1), - div( - // Still not updated. - span(0), - span(0), - span(0), - ), - ), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( +
+ {/* Updated */} + + +
, + ); expect(ops).toEqual(['Bar']); ops = []; @@ -907,17 +966,17 @@ describe('ReactIncrementalSideEffects', () => { // However, once we render fully, we will have enough time to finish it all // at once. ReactNoop.flush(); - expect(ReactNoop.getChildren()).toEqual([ - div( - span(1), - div( - // Now we had enough time to finish the spans. - span('X'), - span(1), - span(1), - ), - ), - ]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( +
+ + +
, + ); expect(ops).toEqual(['Bar', 'Bar', 'Bar']); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js index dc25f1e3f3abe..fd82638be869e 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalTriangle-test.internal.js @@ -24,7 +24,7 @@ describe('ReactIncrementalTriangle', () => { }); function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } const FLUSH = 'FLUSH'; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js index e38046a92e2bc..6801480a14797 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js @@ -24,7 +24,7 @@ describe('ReactIncrementalUpdates', () => { }); function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } it('applies updates in order of priority', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 52ee23f5ffa1f..ebe0c8dcefbac 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -25,18 +25,13 @@ describe('ReactNewContext', () => { gen = require('random-seed'); }); - // function div(...children) { - // children = children.map(c => (typeof c === 'string' ? {text: c} : c)); - // return {type: 'div', children, prop: undefined}; - // } - function Text(props) { ReactNoop.yield(props.text); return ; } function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } // We have several ways of reading from context. sharedContextTests runs diff --git a/packages/react-reconciler/src/__tests__/ReactPersistent-test.js b/packages/react-reconciler/src/__tests__/ReactPersistent-test.js index 20f6e776df575..c0e08ef388652 100644 --- a/packages/react-reconciler/src/__tests__/ReactPersistent-test.js +++ b/packages/react-reconciler/src/__tests__/ReactPersistent-test.js @@ -37,12 +37,14 @@ describe('ReactPersistent', () => { } function div(...children) { - children = children.map(c => (typeof c === 'string' ? {text: c} : c)); - return {type: 'div', children, prop: undefined}; + children = children.map( + c => (typeof c === 'string' ? {text: c, hidden: false} : c), + ); + return {type: 'div', children, prop: undefined, hidden: false}; } function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } function getChildren() { diff --git a/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js b/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js index 3d53293599919..13dc75c78df3a 100644 --- a/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js @@ -26,7 +26,7 @@ describe('pure', () => { }); function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } function Text(props) { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 14ce6cc9beaa4..ab0b6e98712ac 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -51,12 +51,14 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); function div(...children) { - children = children.map(c => (typeof c === 'string' ? {text: c} : c)); - return {type: 'div', children, prop: undefined}; + children = children.map( + c => (typeof c === 'string' ? {text: c, hidden: false} : c), + ); + return {type: 'div', children, prop: undefined, hidden: false}; } function span(prop) { - return {type: 'span', children: [], prop}; + return {type: 'span', children: [], prop, hidden: false}; } function advanceTimers(ms) { @@ -788,18 +790,27 @@ describe('ReactSuspenseWithNoopRenderer', () => { ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['Suspend! [Async]', 'Loading...']); - expect(ReactNoop.getChildren()).toEqual([]); + expect(ReactNoop.getChildrenAsJSX()).toEqual(null); ReactNoop.expire(2000); await advanceTimers(2000); expect(ReactNoop.flush()).toEqual([]); - expect(ReactNoop.getChildren()).toEqual([div(), span('Loading...')]); + expect(ReactNoop.getChildrenAsJSX()).toEqual( + + }> +
+ + {this.props.children} +
+ + + + ); + } +} + +class SuspendyTree extends React.Component { + parentContainer = React.createRef(null); + container = React.createRef(null); + componentDidMount() { + this.setState({}); + document.addEventListener('keydown', this.onKeydown); + } + componentWillUnmount() { + document.removeEventListener('keydown', this.onKeydown); + } + onKeydown = event => { + if (event.metaKey && event.key === '/') { + this.removeAndRestore(); + } + }; + removeAndRestore = () => { + const parentContainer = this.parentContainer.current; + const container = this.container.current; + parentContainer.removeChild(container); + parentContainer.textContent = '(removed from DOM)'; + setTimeout(() => { + parentContainer.textContent = ''; + parentContainer.appendChild(container); + }, 500); + }; + render() { + return ( + +
+
+
+
+ {this.container.current !== null + ? ReactDOM.createPortal( + + {this.props.children} + + , + this.container.current + ) + : null} +
+ + ); + } +} + +class TextInputFixtures extends React.Component { + render() { + return ( + +

+ Clicking "Hide" will hide the fixture context using{' '} + display: none for 0.5 seconds, then restore. This is the + built-in behavior for timed-out children. Each fixture tests whether + the state of the DOM is preserved. Clicking "Remove" will remove the + fixture content from the DOM for 0.5 seconds, then restore. This is{' '} + not how timed-out children are hidden, but is + included for comparison purposes. +

+
+ As a shortcut, you can use Command + Enter (or Control + Enter on + Windows, Linux) to "Hide" all the fixtures, or Command + / to "Remove" + them. +
+ + +
  • Use your cursor to select the text below.
    • +
  • Click "Hide" or "Remove".
    • +     + + + Text selection is preserved when hiding, but not when removing. + + + + + Select this entire sentence (and only this sentence). + + +     + + +
  • + Use your cursor to select a range that includes both the text and + the "Go" button. +
    • +
  • Click "Hide" or "Remove".
    • +     + + + Text selection is preserved when hiding, but not when removing. + + + + + Select a range that includes both this sentence and the "Go" + button. + + +     + + +
  • + Use your cursor to select a range that includes both the text and + the "Go" button. +
    • +
  • + Intead of clicking "Go", which switches focus, press Command + + Enter (or Control + Enter on Windows, Linux). +
    • +     + + + The ideal behavior is that the focus would not be lost, but + currently it is (both when hiding and removing). + + + + + + + +     + + +
  • Type something ("Hello") into the text input.
    • +
  • Click "Hide" or "Remove".
    • +     + + + Input is preserved when hiding, but not when removing. + + + + + + + +     + + +
  • Click "Hide" or "Remove".
    • +     + + + The image should reappear without flickering. The text should not + reflow. + + + + + React + is cool + + +     + + +
  • + The iframe shows a nested version of this fixtures app. Navigate + to the "Text inputs" page. +
    • +
  • Select one of the checkboxes.
    • +
  • Click "Hide" or "Remove".
    • +     + + + When removing, the iframe is reloaded. When hiding, the iframe + should still be on the "Text inputs" page. The checkbox should still + be checked. (Unfortunately, scroll position is lost.) + + + + +