From cefba1bd7b7e373e131304bf982e0ec070539f6b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 22 Oct 2022 22:52:39 -0400 Subject: [PATCH 1/3] Revert yieldy behavior for non-use Suspense (in Flight, too) Same as #25537 but for Flight. I was going to wait to do this later because the temporary implementation of async components uses some of the same code that non-used wakables do, but it's not so bad. I just had to inline one bit of code, which we'll remove when we unify the implementation with `use`. --- .../react-server/src/ReactFlightServer.js | 49 +++++++++++++++---- .../react-server/src/ReactFlightThenable.js | 29 +++-------- 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index dc82d78d0d40d..b91b8db2a9694 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -22,6 +22,9 @@ import type { ServerContextJSONValue, Wakeable, Thenable, + PendingThenable, + FulfilledThenable, + RejectedThenable, } from 'shared/ReactTypes'; import type {LazyComponent} from 'react/src/ReactLazy'; @@ -66,7 +69,6 @@ import { getActiveContext, rootContextSnapshot, } from './ReactFlightNewContext'; -import {trackSuspendedWakeable} from './ReactFlightThenable'; import { REACT_ELEMENT_TYPE, @@ -224,10 +226,44 @@ function readThenable(thenable: Thenable): T { } function createLazyWrapperAroundWakeable(wakeable: Wakeable) { - trackSuspendedWakeable(wakeable); + // This is a temporary fork of the `use` implementation until we accept + // promises everywhere. + const thenable: Thenable = (wakeable: any); + switch (thenable.status) { + case 'fulfilled': + case 'rejected': + break; + default: { + if (typeof thenable.status === 'string') { + // Only instrument the thenable if the status if not defined. If + // it's defined, but an unknown value, assume it's been instrumented by + // some custom userspace implementation. We treat it as "pending". + break; + } + const pendingThenable: PendingThenable = (thenable: any); + pendingThenable.status = 'pending'; + pendingThenable.then( + fulfilledValue => { + if (thenable.status === 'pending') { + const fulfilledThenable: FulfilledThenable = (thenable: any); + fulfilledThenable.status = 'fulfilled'; + fulfilledThenable.value = fulfilledValue; + } + }, + (error: mixed) => { + if (thenable.status === 'pending') { + const rejectedThenable: RejectedThenable = (thenable: any); + rejectedThenable.status = 'rejected'; + rejectedThenable.reason = error; + } + }, + ); + break; + } + } const lazyType: LazyComponent> = { $$typeof: REACT_LAZY_TYPE, - _payload: (wakeable: any), + _payload: thenable, _init: readThenable, }; return lazyType; @@ -818,11 +854,7 @@ export function resolveModelToJSON( ); const ping = newTask.ping; x.then(ping, ping); - - const wakeable: Wakeable = x; - trackSuspendedWakeable(wakeable); newTask.thenableState = getThenableStateAfterSuspending(); - return serializeByRefID(newTask.id); } else { // Something errored. We'll still send everything we have up until this point. @@ -1146,9 +1178,6 @@ function retryTask(request: Request, task: Task): void { // Something suspended again, let's pick it back up later. const ping = task.ping; x.then(ping, ping); - - const wakeable: Wakeable = x; - trackSuspendedWakeable(wakeable); task.thenableState = getThenableStateAfterSuspending(); return; } else { diff --git a/packages/react-server/src/ReactFlightThenable.js b/packages/react-server/src/ReactFlightThenable.js index ac47b76bc1acb..eaeb635217d68 100644 --- a/packages/react-server/src/ReactFlightThenable.js +++ b/packages/react-server/src/ReactFlightThenable.js @@ -14,7 +14,6 @@ // instead of "Wakeable". Or some other more appropriate name. import type { - Wakeable, Thenable, PendingThenable, FulfilledThenable, @@ -30,14 +29,12 @@ export function createThenableState(): ThenableState { return []; } -// TODO: Unify this with trackSuspendedThenable. It needs to support not only -// `use`, but async components, too. -export function trackSuspendedWakeable(wakeable: Wakeable) { - // If this wakeable isn't already a thenable, turn it into one now. Then, - // when we resume the work loop, we can check if its status is - // still pending. - // TODO: Get rid of the Wakeable type? It's superseded by UntrackedThenable. - const thenable: Thenable = (wakeable: any); +export function trackUsedThenable( + thenableState: ThenableState, + thenable: Thenable, + index: number, +) { + thenableState[index] = thenable; // We use an expando to track the status and result of a thenable so that we // can synchronously unwrap the value. Think of this as an extension of the @@ -84,20 +81,6 @@ export function trackSuspendedWakeable(wakeable: Wakeable) { } } -export function trackUsedThenable( - thenableState: ThenableState, - thenable: Thenable, - index: number, -) { - // This is only a separate function from trackSuspendedWakeable for symmetry - // with Fiber. - // TODO: Disallow throwing a thenable directly. It must go through `use` (or - // some equivalent for internal Suspense implementations). We can't do this in - // Fiber yet because it's a breaking change but we can do it in Server - // Components because Server Components aren't released yet. - thenableState[index] = thenable; -} - export function getPreviouslyUsedThenableAtIndex( thenableState: ThenableState | null, index: number, From 6ad6cdf5f55c0dd3b67c1360142626c0e1297588 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 22 Oct 2022 19:29:58 -0400 Subject: [PATCH 2/3] Track thenable state in work loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a refactor to track the array of thenables that is preserved across replays in the work loop instead of the Thenable module. The reason is that I'm about to add additional state to the Thenable module that is specific to a particular attempt — like the current index — and is reset between replays. So it's helpful to keep the two kinds of state separate so it's clearer which state gets reset when. The array of thenables is not reset until the work-in-progress either completes or unwinds. This also makes the structure more similar to Fizz and Flight. --- .../src/ReactFiberHooks.new.js | 6 ++ .../src/ReactFiberHooks.old.js | 6 ++ .../src/ReactFiberThenable.new.js | 67 ++++++++++------- .../src/ReactFiberThenable.old.js | 67 ++++++++++------- .../src/ReactFiberWorkLoop.new.js | 27 ++++--- .../src/ReactFiberWorkLoop.old.js | 27 ++++--- .../src/__tests__/ReactThenable-test.js | 72 ++++++++++++++++++- 7 files changed, 201 insertions(+), 71 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 2a358efc0a5a8..11c6a61f83483 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -102,6 +102,7 @@ import { requestEventTime, markSkippedUpdateLanes, isInvalidExecutionContextForEventFunction, + getSuspendedThenableState, } from './ReactFiberWorkLoop.new'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; @@ -134,6 +135,7 @@ import { import {getTreeId} from './ReactFiberTreeContext.new'; import {now} from './Scheduler'; import { + prepareThenableState, trackUsedThenable, getPreviouslyUsedThenableAtIndex, } from './ReactFiberThenable.new'; @@ -465,6 +467,9 @@ export function renderWithHooks( : HooksDispatcherOnUpdate; } + // If this is a replay, restore the thenable state from the previous attempt. + const prevThenableState = getSuspendedThenableState(); + prepareThenableState(prevThenableState); let children = Component(props, secondArg); // Check if there was a render phase update @@ -506,6 +511,7 @@ export function renderWithHooks( ? HooksDispatcherOnRerenderInDEV : HooksDispatcherOnRerender; + prepareThenableState(prevThenableState); children = Component(props, secondArg); } while (didScheduleRenderPhaseUpdateDuringThisPass); } diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 3370f51cbd412..705e8b659c2ea 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -102,6 +102,7 @@ import { requestEventTime, markSkippedUpdateLanes, isInvalidExecutionContextForEventFunction, + getSuspendedThenableState, } from './ReactFiberWorkLoop.old'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; @@ -134,6 +135,7 @@ import { import {getTreeId} from './ReactFiberTreeContext.old'; import {now} from './Scheduler'; import { + prepareThenableState, trackUsedThenable, getPreviouslyUsedThenableAtIndex, } from './ReactFiberThenable.old'; @@ -465,6 +467,9 @@ export function renderWithHooks( : HooksDispatcherOnUpdate; } + // If this is a replay, restore the thenable state from the previous attempt. + const prevThenableState = getSuspendedThenableState(); + prepareThenableState(prevThenableState); let children = Component(props, secondArg); // Check if there was a render phase update @@ -506,6 +511,7 @@ export function renderWithHooks( ? HooksDispatcherOnRerenderInDEV : HooksDispatcherOnRerender; + prepareThenableState(prevThenableState); children = Component(props, secondArg); } while (didScheduleRenderPhaseUpdateDuringThisPass); } diff --git a/packages/react-reconciler/src/ReactFiberThenable.new.js b/packages/react-reconciler/src/ReactFiberThenable.new.js index 0a8f9d8deaad6..a7b958006a332 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.new.js +++ b/packages/react-reconciler/src/ReactFiberThenable.new.js @@ -17,19 +17,49 @@ import type { import ReactSharedInternals from 'shared/ReactSharedInternals'; const {ReactCurrentActQueue} = ReactSharedInternals; -let suspendedThenable: Thenable | null = null; -let usedThenables: Array | void> | null = null; +// TODO: Sparse arrays are bad for performance. +export opaque type ThenableState = Array | void>; -export function isTrackingSuspendedThenable(): boolean { - return suspendedThenable !== null; +let thenableState: ThenableState | null = null; + +export function createThenableState(): ThenableState { + // The ThenableState is created the first time a component suspends. If it + // suspends again, we'll reuse the same state. + return []; +} + +export function prepareThenableState(prevThenableState: ThenableState | null) { + // This function is called before every function that might suspend + // with `use`. Right now, that's only Hooks, but in the future we'll use the + // same mechanism for unwrapping promises during reconciliation. + thenableState = prevThenableState; +} + +export function getThenableStateAfterSuspending(): ThenableState | null { + // Called by the work loop so it can stash the thenable state. It will use + // the state to replay the component when the promise resolves. + if ( + thenableState !== null && + // If we only `use`-ed resolved promises, then there is no suspended state + // TODO: The only reason we do this is to distinguish between throwing a + // promise (old Suspense pattern) versus `use`-ing one. A better solution is + // for `use` to throw a special, opaque value instead of a promise. + !isThenableStateResolved(thenableState) + ) { + const state = thenableState; + thenableState = null; + return state; + } + return null; } -export function suspendedThenableDidResolve(): boolean { - if (suspendedThenable !== null) { - const status = suspendedThenable.status; +export function isThenableStateResolved(thenables: ThenableState): boolean { + const lastThenable = thenables[thenables.length - 1]; + if (lastThenable !== undefined) { + const status = lastThenable.status; return status === 'fulfilled' || status === 'rejected'; } - return false; + return true; } export function trackUsedThenable(thenable: Thenable, index: number) { @@ -37,14 +67,12 @@ export function trackUsedThenable(thenable: Thenable, index: number) { ReactCurrentActQueue.didUsePromise = true; } - if (usedThenables === null) { - usedThenables = [thenable]; + if (thenableState === null) { + thenableState = [thenable]; } else { - usedThenables[index] = thenable; + thenableState[index] = thenable; } - suspendedThenable = thenable; - // We use an expando to track the status and result of a thenable so that we // can synchronously unwrap the value. Think of this as an extension of the // Promise API, or a custom interface that is a superset of Thenable. @@ -59,7 +87,6 @@ export function trackUsedThenable(thenable: Thenable, index: number) { // this thenable, because if we keep trying it will likely infinite loop // without ever resolving. // TODO: Log a warning? - suspendedThenable = null; break; default: { if (typeof thenable.status === 'string') { @@ -91,19 +118,11 @@ export function trackUsedThenable(thenable: Thenable, index: number) { } } -export function resetWakeableStateAfterEachAttempt() { - suspendedThenable = null; -} - -export function resetThenableStateOnCompletion() { - usedThenables = null; -} - export function getPreviouslyUsedThenableAtIndex( index: number, ): Thenable | null { - if (usedThenables !== null) { - const thenable = usedThenables[index]; + if (thenableState !== null) { + const thenable = thenableState[index]; if (thenable !== undefined) { return thenable; } diff --git a/packages/react-reconciler/src/ReactFiberThenable.old.js b/packages/react-reconciler/src/ReactFiberThenable.old.js index 0a8f9d8deaad6..a7b958006a332 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.old.js +++ b/packages/react-reconciler/src/ReactFiberThenable.old.js @@ -17,19 +17,49 @@ import type { import ReactSharedInternals from 'shared/ReactSharedInternals'; const {ReactCurrentActQueue} = ReactSharedInternals; -let suspendedThenable: Thenable | null = null; -let usedThenables: Array | void> | null = null; +// TODO: Sparse arrays are bad for performance. +export opaque type ThenableState = Array | void>; -export function isTrackingSuspendedThenable(): boolean { - return suspendedThenable !== null; +let thenableState: ThenableState | null = null; + +export function createThenableState(): ThenableState { + // The ThenableState is created the first time a component suspends. If it + // suspends again, we'll reuse the same state. + return []; +} + +export function prepareThenableState(prevThenableState: ThenableState | null) { + // This function is called before every function that might suspend + // with `use`. Right now, that's only Hooks, but in the future we'll use the + // same mechanism for unwrapping promises during reconciliation. + thenableState = prevThenableState; +} + +export function getThenableStateAfterSuspending(): ThenableState | null { + // Called by the work loop so it can stash the thenable state. It will use + // the state to replay the component when the promise resolves. + if ( + thenableState !== null && + // If we only `use`-ed resolved promises, then there is no suspended state + // TODO: The only reason we do this is to distinguish between throwing a + // promise (old Suspense pattern) versus `use`-ing one. A better solution is + // for `use` to throw a special, opaque value instead of a promise. + !isThenableStateResolved(thenableState) + ) { + const state = thenableState; + thenableState = null; + return state; + } + return null; } -export function suspendedThenableDidResolve(): boolean { - if (suspendedThenable !== null) { - const status = suspendedThenable.status; +export function isThenableStateResolved(thenables: ThenableState): boolean { + const lastThenable = thenables[thenables.length - 1]; + if (lastThenable !== undefined) { + const status = lastThenable.status; return status === 'fulfilled' || status === 'rejected'; } - return false; + return true; } export function trackUsedThenable(thenable: Thenable, index: number) { @@ -37,14 +67,12 @@ export function trackUsedThenable(thenable: Thenable, index: number) { ReactCurrentActQueue.didUsePromise = true; } - if (usedThenables === null) { - usedThenables = [thenable]; + if (thenableState === null) { + thenableState = [thenable]; } else { - usedThenables[index] = thenable; + thenableState[index] = thenable; } - suspendedThenable = thenable; - // We use an expando to track the status and result of a thenable so that we // can synchronously unwrap the value. Think of this as an extension of the // Promise API, or a custom interface that is a superset of Thenable. @@ -59,7 +87,6 @@ export function trackUsedThenable(thenable: Thenable, index: number) { // this thenable, because if we keep trying it will likely infinite loop // without ever resolving. // TODO: Log a warning? - suspendedThenable = null; break; default: { if (typeof thenable.status === 'string') { @@ -91,19 +118,11 @@ export function trackUsedThenable(thenable: Thenable, index: number) { } } -export function resetWakeableStateAfterEachAttempt() { - suspendedThenable = null; -} - -export function resetThenableStateOnCompletion() { - usedThenables = null; -} - export function getPreviouslyUsedThenableAtIndex( index: number, ): Thenable | null { - if (usedThenables !== null) { - const thenable = usedThenables[index]; + if (thenableState !== null) { + const thenable = thenableState[index]; if (thenable !== undefined) { return thenable; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 32229d31851cd..d94060cdac43c 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -22,6 +22,7 @@ import type { TransitionAbort, } from './ReactFiberTracingMarkerComponent.new'; import type {OffscreenInstance} from './ReactFiberOffscreenComponent'; +import type {ThenableState} from './ReactFiberThenable.new'; import { warnAboutDeprecatedLifecycles, @@ -265,10 +266,8 @@ import { } from './ReactFiberAct.new'; import {processTransitionCallbacks} from './ReactFiberTracingMarkerComponent.new'; import { - resetWakeableStateAfterEachAttempt, - resetThenableStateOnCompletion, - suspendedThenableDidResolve, - isTrackingSuspendedThenable, + getThenableStateAfterSuspending, + isThenableStateResolved, } from './ReactFiberThenable.new'; import {schedulePostPaintCallback} from './ReactPostPaintCallback'; @@ -315,6 +314,7 @@ let workInProgressRootRenderLanes: Lanes = NoLanes; // immediately instead of unwinding the stack. let workInProgressIsSuspended: boolean = false; let workInProgressThrownValue: mixed = null; +let workInProgressSuspendedThenableState: ThenableState | null = null; // Whether a ping listener was attached during this render. This is slightly // different that whether something suspended, because we don't add multiple @@ -1686,8 +1686,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { ); interruptedWork = interruptedWork.return; } - resetWakeableStateAfterEachAttempt(); - resetThenableStateOnCompletion(); } workInProgressRoot = root; const rootWorkInProgress = createWorkInProgress(root.current, null); @@ -1695,6 +1693,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { workInProgressRootRenderLanes = renderLanes = lanes; workInProgressIsSuspended = false; workInProgressThrownValue = null; + workInProgressSuspendedThenableState = null; workInProgressRootDidAttachPingListener = false; workInProgressRootExitStatus = RootInProgress; workInProgressRootFatalError = null; @@ -1729,6 +1728,7 @@ function handleThrow(root, thrownValue): void { // as suspending the execution of the work loop. workInProgressIsSuspended = true; workInProgressThrownValue = thrownValue; + workInProgressSuspendedThenableState = getThenableStateAfterSuspending(); const erroredWork = workInProgress; if (erroredWork === null) { @@ -2014,7 +2014,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { break; } catch (thrownValue) { handleThrow(root, thrownValue); - if (isTrackingSuspendedThenable()) { + if (workInProgressSuspendedThenableState !== null) { // If this fiber just suspended, it's possible the data is already // cached. Yield to the main thread to give it a chance to ping. If // it does, we can retry immediately without unwinding the stack. @@ -2117,13 +2117,14 @@ function resumeSuspendedUnitOfWork( // instead of unwinding the stack. It's a separate function to keep the // additional logic out of the work loop's hot path. - const wasPinged = suspendedThenableDidResolve(); - resetWakeableStateAfterEachAttempt(); + const wasPinged = + workInProgressSuspendedThenableState !== null && + isThenableStateResolved(workInProgressSuspendedThenableState); if (!wasPinged) { // The thenable wasn't pinged. Return to the normal work loop. This will // unwind the stack, and potentially result in showing a fallback. - resetThenableStateOnCompletion(); + workInProgressSuspendedThenableState = null; const returnFiber = unitOfWork.return; if (returnFiber === null || workInProgressRoot === null) { @@ -2188,7 +2189,7 @@ function resumeSuspendedUnitOfWork( // The begin phase finished successfully without suspending. Reset the state // used to track the fiber while it was suspended. Then return to the normal // work loop. - resetThenableStateOnCompletion(); + workInProgressSuspendedThenableState = null; resetCurrentDebugFiberInDEV(); unitOfWork.memoizedProps = unitOfWork.pendingProps; @@ -2202,6 +2203,10 @@ function resumeSuspendedUnitOfWork( ReactCurrentOwner.current = null; } +export function getSuspendedThenableState(): ThenableState | null { + return workInProgressSuspendedThenableState; +} + function completeUnitOfWork(unitOfWork: Fiber): void { // Attempt to complete the current unit of work, then move to the next // sibling. If there are no more siblings, return to the parent fiber. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 4c86b9707b452..ae24f0f5af572 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -22,6 +22,7 @@ import type { TransitionAbort, } from './ReactFiberTracingMarkerComponent.old'; import type {OffscreenInstance} from './ReactFiberOffscreenComponent'; +import type {ThenableState} from './ReactFiberThenable.old'; import { warnAboutDeprecatedLifecycles, @@ -265,10 +266,8 @@ import { } from './ReactFiberAct.old'; import {processTransitionCallbacks} from './ReactFiberTracingMarkerComponent.old'; import { - resetWakeableStateAfterEachAttempt, - resetThenableStateOnCompletion, - suspendedThenableDidResolve, - isTrackingSuspendedThenable, + getThenableStateAfterSuspending, + isThenableStateResolved, } from './ReactFiberThenable.old'; import {schedulePostPaintCallback} from './ReactPostPaintCallback'; @@ -315,6 +314,7 @@ let workInProgressRootRenderLanes: Lanes = NoLanes; // immediately instead of unwinding the stack. let workInProgressIsSuspended: boolean = false; let workInProgressThrownValue: mixed = null; +let workInProgressSuspendedThenableState: ThenableState | null = null; // Whether a ping listener was attached during this render. This is slightly // different that whether something suspended, because we don't add multiple @@ -1686,8 +1686,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { ); interruptedWork = interruptedWork.return; } - resetWakeableStateAfterEachAttempt(); - resetThenableStateOnCompletion(); } workInProgressRoot = root; const rootWorkInProgress = createWorkInProgress(root.current, null); @@ -1695,6 +1693,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { workInProgressRootRenderLanes = renderLanes = lanes; workInProgressIsSuspended = false; workInProgressThrownValue = null; + workInProgressSuspendedThenableState = null; workInProgressRootDidAttachPingListener = false; workInProgressRootExitStatus = RootInProgress; workInProgressRootFatalError = null; @@ -1729,6 +1728,7 @@ function handleThrow(root, thrownValue): void { // as suspending the execution of the work loop. workInProgressIsSuspended = true; workInProgressThrownValue = thrownValue; + workInProgressSuspendedThenableState = getThenableStateAfterSuspending(); const erroredWork = workInProgress; if (erroredWork === null) { @@ -2014,7 +2014,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { break; } catch (thrownValue) { handleThrow(root, thrownValue); - if (isTrackingSuspendedThenable()) { + if (workInProgressSuspendedThenableState !== null) { // If this fiber just suspended, it's possible the data is already // cached. Yield to the main thread to give it a chance to ping. If // it does, we can retry immediately without unwinding the stack. @@ -2117,13 +2117,14 @@ function resumeSuspendedUnitOfWork( // instead of unwinding the stack. It's a separate function to keep the // additional logic out of the work loop's hot path. - const wasPinged = suspendedThenableDidResolve(); - resetWakeableStateAfterEachAttempt(); + const wasPinged = + workInProgressSuspendedThenableState !== null && + isThenableStateResolved(workInProgressSuspendedThenableState); if (!wasPinged) { // The thenable wasn't pinged. Return to the normal work loop. This will // unwind the stack, and potentially result in showing a fallback. - resetThenableStateOnCompletion(); + workInProgressSuspendedThenableState = null; const returnFiber = unitOfWork.return; if (returnFiber === null || workInProgressRoot === null) { @@ -2188,7 +2189,7 @@ function resumeSuspendedUnitOfWork( // The begin phase finished successfully without suspending. Reset the state // used to track the fiber while it was suspended. Then return to the normal // work loop. - resetThenableStateOnCompletion(); + workInProgressSuspendedThenableState = null; resetCurrentDebugFiberInDEV(); unitOfWork.memoizedProps = unitOfWork.pendingProps; @@ -2202,6 +2203,10 @@ function resumeSuspendedUnitOfWork( ReactCurrentOwner.current = null; } +export function getSuspendedThenableState(): ThenableState | null { + return workInProgressSuspendedThenableState; +} + function completeUnitOfWork(unitOfWork: Fiber): void { // Attempt to complete the current unit of work, then move to the next // sibling. If there are no more siblings, return to the parent fiber. diff --git a/packages/react-reconciler/src/__tests__/ReactThenable-test.js b/packages/react-reconciler/src/__tests__/ReactThenable-test.js index 5062ff71658ca..2ac5c8a21995a 100644 --- a/packages/react-reconciler/src/__tests__/ReactThenable-test.js +++ b/packages/react-reconciler/src/__tests__/ReactThenable-test.js @@ -8,7 +8,7 @@ let use; let Suspense; let startTransition; -describe('ReactWakeable', () => { +describe('ReactThenable', () => { beforeEach(() => { jest.resetModules(); @@ -243,6 +243,76 @@ describe('ReactWakeable', () => { expect(Scheduler).toHaveYielded(['Oops!', 'Oops!']); }); + // @gate enableUseHook + test('use(promise) in multiple components', async () => { + // This tests that the state for tracking promises is reset per component. + const promiseA = Promise.resolve('A'); + const promiseB = Promise.resolve('B'); + const promiseC = Promise.resolve('C'); + const promiseD = Promise.resolve('D'); + + function Child({prefix}) { + return ; + } + + function Parent() { + return ; + } + + function App() { + return ( + }> + + + ); + } + + const root = ReactNoop.createRoot(); + await act(async () => { + startTransition(() => { + root.render(); + }); + }); + expect(Scheduler).toHaveYielded(['ABCD']); + expect(root).toMatchRenderedOutput('ABCD'); + }); + + // @gate enableUseHook + test('use(promise) in multiple sibling components', async () => { + // This tests that the state for tracking promises is reset per component. + + const promiseA = {then: () => {}, status: 'pending', value: null}; + const promiseB = {then: () => {}, status: 'pending', value: null}; + const promiseC = {then: () => {}, status: 'fulfilled', value: 'C'}; + const promiseD = {then: () => {}, status: 'fulfilled', value: 'D'}; + + function Sibling1({prefix}) { + return ; + } + + function Sibling2() { + return ; + } + + function App() { + return ( + }> + + + + ); + } + + const root = ReactNoop.createRoot(); + await act(async () => { + startTransition(() => { + root.render(); + }); + }); + expect(Scheduler).toHaveYielded(['CD', 'Loading...']); + expect(root).toMatchRenderedOutput('Loading...'); + }); + // @gate enableUseHook test('erroring in the same component as an uncached promise does not result in an infinite loop', async () => { class ErrorBoundary extends React.Component { From 66121957933d03a738ab3e688eb72fd91e038095 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 22 Oct 2022 20:39:19 -0400 Subject: [PATCH 3/3] Unify promise switch statements There are two different switch statements that we use to unwrap a `use`-ed promise, but there really only needs to be one. This was a factoring artifact that arose because I implemented the yieldy `status` instrumentation thing before I implemented `use` (for promises that are thrown directly during render, which is the old Suspense pattern that will be superseded by `use`). --- .../src/ReactFiberHooks.new.js | 57 +---------- .../src/ReactFiberHooks.old.js | 57 +---------- .../src/ReactFiberThenable.new.js | 96 ++++++++++-------- .../src/ReactFiberThenable.old.js | 96 ++++++++++-------- packages/react-server/src/ReactFizzHooks.js | 64 +----------- .../react-server/src/ReactFizzThenable.js | 97 ++++++++++--------- packages/react-server/src/ReactFlightHooks.js | 65 +------------ .../react-server/src/ReactFlightThenable.js | 97 ++++++++++--------- 8 files changed, 222 insertions(+), 407 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 11c6a61f83483..0b6c540ecc741 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -137,7 +137,6 @@ import {now} from './Scheduler'; import { prepareThenableState, trackUsedThenable, - getPreviouslyUsedThenableAtIndex, } from './ReactFiberThenable.new'; const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals; @@ -776,8 +775,6 @@ if (enableUseMemoCacheHook) { }; } -function noop(): void {} - function use(usable: Usable): T { if (usable !== null && typeof usable === 'object') { // $FlowFixMe[method-unbinding] @@ -788,59 +785,7 @@ function use(usable: Usable): T { // Track the position of the thenable within this fiber. const index = thenableIndexCounter; thenableIndexCounter += 1; - - // TODO: Unify this switch statement with the one in trackUsedThenable. - switch (thenable.status) { - case 'fulfilled': { - const fulfilledValue: T = thenable.value; - return fulfilledValue; - } - case 'rejected': { - const rejectedError = thenable.reason; - throw rejectedError; - } - default: { - const prevThenableAtIndex: Thenable | null = getPreviouslyUsedThenableAtIndex( - index, - ); - if (prevThenableAtIndex !== null) { - if (thenable !== prevThenableAtIndex) { - // Avoid an unhandled rejection errors for the Promises that we'll - // intentionally ignore. - thenable.then(noop, noop); - } - switch (prevThenableAtIndex.status) { - case 'fulfilled': { - const fulfilledValue: T = prevThenableAtIndex.value; - return fulfilledValue; - } - case 'rejected': { - const rejectedError: mixed = prevThenableAtIndex.reason; - throw rejectedError; - } - default: { - // The thenable still hasn't resolved. Suspend with the same - // thenable as last time to avoid redundant listeners. - throw prevThenableAtIndex; - } - } - } else { - // This is the first time something has been used at this index. - // Stash the thenable at the current index so we can reuse it during - // the next attempt. - trackUsedThenable(thenable, index); - - // Suspend. - // TODO: Throwing here is an implementation detail that allows us to - // unwind the call stack. But we shouldn't allow it to leak into - // userspace. Throw an opaque placeholder value instead of the - // actual thenable. If it doesn't get captured by the work loop, log - // a warning, because that means something in userspace must have - // caught it. - throw thenable; - } - } - } + return trackUsedThenable(thenable, index); } else if ( usable.$$typeof === REACT_CONTEXT_TYPE || usable.$$typeof === REACT_SERVER_CONTEXT_TYPE diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 705e8b659c2ea..cb5eeb4adb459 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -137,7 +137,6 @@ import {now} from './Scheduler'; import { prepareThenableState, trackUsedThenable, - getPreviouslyUsedThenableAtIndex, } from './ReactFiberThenable.old'; const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals; @@ -776,8 +775,6 @@ if (enableUseMemoCacheHook) { }; } -function noop(): void {} - function use(usable: Usable): T { if (usable !== null && typeof usable === 'object') { // $FlowFixMe[method-unbinding] @@ -788,59 +785,7 @@ function use(usable: Usable): T { // Track the position of the thenable within this fiber. const index = thenableIndexCounter; thenableIndexCounter += 1; - - // TODO: Unify this switch statement with the one in trackUsedThenable. - switch (thenable.status) { - case 'fulfilled': { - const fulfilledValue: T = thenable.value; - return fulfilledValue; - } - case 'rejected': { - const rejectedError = thenable.reason; - throw rejectedError; - } - default: { - const prevThenableAtIndex: Thenable | null = getPreviouslyUsedThenableAtIndex( - index, - ); - if (prevThenableAtIndex !== null) { - if (thenable !== prevThenableAtIndex) { - // Avoid an unhandled rejection errors for the Promises that we'll - // intentionally ignore. - thenable.then(noop, noop); - } - switch (prevThenableAtIndex.status) { - case 'fulfilled': { - const fulfilledValue: T = prevThenableAtIndex.value; - return fulfilledValue; - } - case 'rejected': { - const rejectedError: mixed = prevThenableAtIndex.reason; - throw rejectedError; - } - default: { - // The thenable still hasn't resolved. Suspend with the same - // thenable as last time to avoid redundant listeners. - throw prevThenableAtIndex; - } - } - } else { - // This is the first time something has been used at this index. - // Stash the thenable at the current index so we can reuse it during - // the next attempt. - trackUsedThenable(thenable, index); - - // Suspend. - // TODO: Throwing here is an implementation detail that allows us to - // unwind the call stack. But we shouldn't allow it to leak into - // userspace. Throw an opaque placeholder value instead of the - // actual thenable. If it doesn't get captured by the work loop, log - // a warning, because that means something in userspace must have - // caught it. - throw thenable; - } - } - } + return trackUsedThenable(thenable, index); } else if ( usable.$$typeof === REACT_CONTEXT_TYPE || usable.$$typeof === REACT_SERVER_CONTEXT_TYPE diff --git a/packages/react-reconciler/src/ReactFiberThenable.new.js b/packages/react-reconciler/src/ReactFiberThenable.new.js index a7b958006a332..c2de159f89ca9 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.new.js +++ b/packages/react-reconciler/src/ReactFiberThenable.new.js @@ -17,8 +17,7 @@ import type { import ReactSharedInternals from 'shared/ReactSharedInternals'; const {ReactCurrentActQueue} = ReactSharedInternals; -// TODO: Sparse arrays are bad for performance. -export opaque type ThenableState = Array | void>; +export opaque type ThenableState = Array>; let thenableState: ThenableState | null = null; @@ -62,7 +61,9 @@ export function isThenableStateResolved(thenables: ThenableState): boolean { return true; } -export function trackUsedThenable(thenable: Thenable, index: number) { +function noop(): void {} + +export function trackUsedThenable(thenable: Thenable, index: number): T { if (__DEV__ && ReactCurrentActQueue.current !== null) { ReactCurrentActQueue.didUsePromise = true; } @@ -70,7 +71,20 @@ export function trackUsedThenable(thenable: Thenable, index: number) { if (thenableState === null) { thenableState = [thenable]; } else { - thenableState[index] = thenable; + const previous = thenableState[index]; + if (previous === undefined) { + thenableState.push(thenable); + } else { + if (previous !== thenable) { + // Reuse the previous thenable, and drop the new one. We can assume + // they represent the same value, because components are idempotent. + + // Avoid an unhandled rejection errors for the Promises that we'll + // intentionally ignore. + thenable.then(noop, noop); + thenable = previous; + } + } } // We use an expando to track the status and result of a thenable so that we @@ -80,52 +94,48 @@ export function trackUsedThenable(thenable: Thenable, index: number) { // If the thenable doesn't have a status, set it to "pending" and attach // a listener that will update its status and result when it resolves. switch (thenable.status) { - case 'fulfilled': - case 'rejected': - // A thenable that already resolved shouldn't have been thrown, so this is - // unexpected. Suggests a mistake in a userspace data library. Don't track - // this thenable, because if we keep trying it will likely infinite loop - // without ever resolving. - // TODO: Log a warning? - break; + case 'fulfilled': { + const fulfilledValue: T = thenable.value; + return fulfilledValue; + } + case 'rejected': { + const rejectedError = thenable.reason; + throw rejectedError; + } default: { if (typeof thenable.status === 'string') { // Only instrument the thenable if the status if not defined. If // it's defined, but an unknown value, assume it's been instrumented by // some custom userspace implementation. We treat it as "pending". - break; + } else { + const pendingThenable: PendingThenable = (thenable: any); + pendingThenable.status = 'pending'; + pendingThenable.then( + fulfilledValue => { + if (thenable.status === 'pending') { + const fulfilledThenable: FulfilledThenable = (thenable: any); + fulfilledThenable.status = 'fulfilled'; + fulfilledThenable.value = fulfilledValue; + } + }, + (error: mixed) => { + if (thenable.status === 'pending') { + const rejectedThenable: RejectedThenable = (thenable: any); + rejectedThenable.status = 'rejected'; + rejectedThenable.reason = error; + } + }, + ); } - const pendingThenable: PendingThenable = (thenable: any); - pendingThenable.status = 'pending'; - pendingThenable.then( - fulfilledValue => { - if (thenable.status === 'pending') { - const fulfilledThenable: FulfilledThenable = (thenable: any); - fulfilledThenable.status = 'fulfilled'; - fulfilledThenable.value = fulfilledValue; - } - }, - (error: mixed) => { - if (thenable.status === 'pending') { - const rejectedThenable: RejectedThenable = (thenable: any); - rejectedThenable.status = 'rejected'; - rejectedThenable.reason = error; - } - }, - ); - break; - } - } -} -export function getPreviouslyUsedThenableAtIndex( - index: number, -): Thenable | null { - if (thenableState !== null) { - const thenable = thenableState[index]; - if (thenable !== undefined) { - return thenable; + // Suspend. + // TODO: Throwing here is an implementation detail that allows us to + // unwind the call stack. But we shouldn't allow it to leak into + // userspace. Throw an opaque placeholder value instead of the + // actual thenable. If it doesn't get captured by the work loop, log + // a warning, because that means something in userspace must have + // caught it. + throw thenable; } } - return null; } diff --git a/packages/react-reconciler/src/ReactFiberThenable.old.js b/packages/react-reconciler/src/ReactFiberThenable.old.js index a7b958006a332..c2de159f89ca9 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.old.js +++ b/packages/react-reconciler/src/ReactFiberThenable.old.js @@ -17,8 +17,7 @@ import type { import ReactSharedInternals from 'shared/ReactSharedInternals'; const {ReactCurrentActQueue} = ReactSharedInternals; -// TODO: Sparse arrays are bad for performance. -export opaque type ThenableState = Array | void>; +export opaque type ThenableState = Array>; let thenableState: ThenableState | null = null; @@ -62,7 +61,9 @@ export function isThenableStateResolved(thenables: ThenableState): boolean { return true; } -export function trackUsedThenable(thenable: Thenable, index: number) { +function noop(): void {} + +export function trackUsedThenable(thenable: Thenable, index: number): T { if (__DEV__ && ReactCurrentActQueue.current !== null) { ReactCurrentActQueue.didUsePromise = true; } @@ -70,7 +71,20 @@ export function trackUsedThenable(thenable: Thenable, index: number) { if (thenableState === null) { thenableState = [thenable]; } else { - thenableState[index] = thenable; + const previous = thenableState[index]; + if (previous === undefined) { + thenableState.push(thenable); + } else { + if (previous !== thenable) { + // Reuse the previous thenable, and drop the new one. We can assume + // they represent the same value, because components are idempotent. + + // Avoid an unhandled rejection errors for the Promises that we'll + // intentionally ignore. + thenable.then(noop, noop); + thenable = previous; + } + } } // We use an expando to track the status and result of a thenable so that we @@ -80,52 +94,48 @@ export function trackUsedThenable(thenable: Thenable, index: number) { // If the thenable doesn't have a status, set it to "pending" and attach // a listener that will update its status and result when it resolves. switch (thenable.status) { - case 'fulfilled': - case 'rejected': - // A thenable that already resolved shouldn't have been thrown, so this is - // unexpected. Suggests a mistake in a userspace data library. Don't track - // this thenable, because if we keep trying it will likely infinite loop - // without ever resolving. - // TODO: Log a warning? - break; + case 'fulfilled': { + const fulfilledValue: T = thenable.value; + return fulfilledValue; + } + case 'rejected': { + const rejectedError = thenable.reason; + throw rejectedError; + } default: { if (typeof thenable.status === 'string') { // Only instrument the thenable if the status if not defined. If // it's defined, but an unknown value, assume it's been instrumented by // some custom userspace implementation. We treat it as "pending". - break; + } else { + const pendingThenable: PendingThenable = (thenable: any); + pendingThenable.status = 'pending'; + pendingThenable.then( + fulfilledValue => { + if (thenable.status === 'pending') { + const fulfilledThenable: FulfilledThenable = (thenable: any); + fulfilledThenable.status = 'fulfilled'; + fulfilledThenable.value = fulfilledValue; + } + }, + (error: mixed) => { + if (thenable.status === 'pending') { + const rejectedThenable: RejectedThenable = (thenable: any); + rejectedThenable.status = 'rejected'; + rejectedThenable.reason = error; + } + }, + ); } - const pendingThenable: PendingThenable = (thenable: any); - pendingThenable.status = 'pending'; - pendingThenable.then( - fulfilledValue => { - if (thenable.status === 'pending') { - const fulfilledThenable: FulfilledThenable = (thenable: any); - fulfilledThenable.status = 'fulfilled'; - fulfilledThenable.value = fulfilledValue; - } - }, - (error: mixed) => { - if (thenable.status === 'pending') { - const rejectedThenable: RejectedThenable = (thenable: any); - rejectedThenable.status = 'rejected'; - rejectedThenable.reason = error; - } - }, - ); - break; - } - } -} -export function getPreviouslyUsedThenableAtIndex( - index: number, -): Thenable | null { - if (thenableState !== null) { - const thenable = thenableState[index]; - if (thenable !== undefined) { - return thenable; + // Suspend. + // TODO: Throwing here is an implementation detail that allows us to + // unwind the call stack. But we shouldn't allow it to leak into + // userspace. Throw an opaque placeholder value instead of the + // actual thenable. If it doesn't get captured by the work loop, log + // a warning, because that means something in userspace must have + // caught it. + throw thenable; } } - return null; } diff --git a/packages/react-server/src/ReactFizzHooks.js b/packages/react-server/src/ReactFizzHooks.js index 5545d8ef8d46e..b8c1e4c92773b 100644 --- a/packages/react-server/src/ReactFizzHooks.js +++ b/packages/react-server/src/ReactFizzHooks.js @@ -25,11 +25,7 @@ import type {ThenableState} from './ReactFizzThenable'; import {readContext as readContextImpl} from './ReactFizzNewContext'; import {getTreeId} from './ReactFizzTreeContext'; -import { - getPreviouslyUsedThenableAtIndex, - createThenableState, - trackUsedThenable, -} from './ReactFizzThenable'; +import {createThenableState, trackUsedThenable} from './ReactFizzThenable'; import {makeId} from './ReactServerFormatConfig'; @@ -593,62 +589,10 @@ function use(usable: Usable): T { const index = thenableIndexCounter; thenableIndexCounter += 1; - // TODO: Unify this switch statement with the one in trackUsedThenable. - switch (thenable.status) { - case 'fulfilled': { - const fulfilledValue: T = thenable.value; - return fulfilledValue; - } - case 'rejected': { - const rejectedError = thenable.reason; - throw rejectedError; - } - default: { - const prevThenableAtIndex: Thenable | null = getPreviouslyUsedThenableAtIndex( - thenableState, - index, - ); - if (prevThenableAtIndex !== null) { - if (thenable !== prevThenableAtIndex) { - // Avoid an unhandled rejection errors for the Promises that we'll - // intentionally ignore. - thenable.then(noop, noop); - } - switch (prevThenableAtIndex.status) { - case 'fulfilled': { - const fulfilledValue: T = prevThenableAtIndex.value; - return fulfilledValue; - } - case 'rejected': { - const rejectedError: mixed = prevThenableAtIndex.reason; - throw rejectedError; - } - default: { - // The thenable still hasn't resolved. Suspend with the same - // thenable as last time to avoid redundant listeners. - throw prevThenableAtIndex; - } - } - } else { - // This is the first time something has been used at this index. - // Stash the thenable at the current index so we can reuse it during - // the next attempt. - if (thenableState === null) { - thenableState = createThenableState(); - } - trackUsedThenable(thenableState, thenable, index); - - // Suspend. - // TODO: Throwing here is an implementation detail that allows us to - // unwind the call stack. But we shouldn't allow it to leak into - // userspace. Throw an opaque placeholder value instead of the - // actual thenable. If it doesn't get captured by the work loop, log - // a warning, because that means something in userspace must have - // caught it. - throw thenable; - } - } + if (thenableState === null) { + thenableState = createThenableState(); } + return trackUsedThenable(thenableState, thenable, index); } else if ( usable.$$typeof === REACT_CONTEXT_TYPE || usable.$$typeof === REACT_SERVER_CONTEXT_TYPE diff --git a/packages/react-server/src/ReactFizzThenable.js b/packages/react-server/src/ReactFizzThenable.js index 52cedc579e3f0..267ee6036041d 100644 --- a/packages/react-server/src/ReactFizzThenable.js +++ b/packages/react-server/src/ReactFizzThenable.js @@ -20,8 +20,7 @@ import type { RejectedThenable, } from 'shared/ReactTypes'; -// TODO: Sparse arrays are bad for performance. -export opaque type ThenableState = Array | void>; +export opaque type ThenableState = Array>; export function createThenableState(): ThenableState { // The ThenableState is created the first time a component suspends. If it @@ -29,12 +28,27 @@ export function createThenableState(): ThenableState { return []; } +function noop(): void {} + export function trackUsedThenable( thenableState: ThenableState, thenable: Thenable, index: number, -) { - thenableState[index] = thenable; +): T { + const previous = thenableState[index]; + if (previous === undefined) { + thenableState.push(thenable); + } else { + if (previous !== thenable) { + // Reuse the previous thenable, and drop the new one. We can assume + // they represent the same value, because components are idempotent. + + // Avoid an unhandled rejection errors for the Promises that we'll + // intentionally ignore. + thenable.then(noop, noop); + thenable = previous; + } + } // We use an expando to track the status and result of a thenable so that we // can synchronously unwrap the value. Think of this as an extension of the @@ -43,53 +57,48 @@ export function trackUsedThenable( // If the thenable doesn't have a status, set it to "pending" and attach // a listener that will update its status and result when it resolves. switch (thenable.status) { - case 'fulfilled': - case 'rejected': - // A thenable that already resolved shouldn't have been thrown, so this is - // unexpected. Suggests a mistake in a userspace data library. Don't track - // this thenable, because if we keep trying it will likely infinite loop - // without ever resolving. - // TODO: Log a warning? - break; + case 'fulfilled': { + const fulfilledValue: T = thenable.value; + return fulfilledValue; + } + case 'rejected': { + const rejectedError = thenable.reason; + throw rejectedError; + } default: { if (typeof thenable.status === 'string') { // Only instrument the thenable if the status if not defined. If // it's defined, but an unknown value, assume it's been instrumented by // some custom userspace implementation. We treat it as "pending". - break; + } else { + const pendingThenable: PendingThenable = (thenable: any); + pendingThenable.status = 'pending'; + pendingThenable.then( + fulfilledValue => { + if (thenable.status === 'pending') { + const fulfilledThenable: FulfilledThenable = (thenable: any); + fulfilledThenable.status = 'fulfilled'; + fulfilledThenable.value = fulfilledValue; + } + }, + (error: mixed) => { + if (thenable.status === 'pending') { + const rejectedThenable: RejectedThenable = (thenable: any); + rejectedThenable.status = 'rejected'; + rejectedThenable.reason = error; + } + }, + ); } - const pendingThenable: PendingThenable = (thenable: any); - pendingThenable.status = 'pending'; - pendingThenable.then( - fulfilledValue => { - if (thenable.status === 'pending') { - const fulfilledThenable: FulfilledThenable = (thenable: any); - fulfilledThenable.status = 'fulfilled'; - fulfilledThenable.value = fulfilledValue; - } - }, - (error: mixed) => { - if (thenable.status === 'pending') { - const rejectedThenable: RejectedThenable = (thenable: any); - rejectedThenable.status = 'rejected'; - rejectedThenable.reason = error; - } - }, - ); - break; - } - } -} -export function getPreviouslyUsedThenableAtIndex( - thenableState: ThenableState | null, - index: number, -): Thenable | null { - if (thenableState !== null) { - const thenable = thenableState[index]; - if (thenable !== undefined) { - return thenable; + // Suspend. + // TODO: Throwing here is an implementation detail that allows us to + // unwind the call stack. But we shouldn't allow it to leak into + // userspace. Throw an opaque placeholder value instead of the + // actual thenable. If it doesn't get captured by the work loop, log + // a warning, because that means something in userspace must have + // caught it. + throw thenable; } } - return null; } diff --git a/packages/react-server/src/ReactFlightHooks.js b/packages/react-server/src/ReactFlightHooks.js index 0f7f5b17e9a68..d1fb683260620 100644 --- a/packages/react-server/src/ReactFlightHooks.js +++ b/packages/react-server/src/ReactFlightHooks.js @@ -17,11 +17,7 @@ import { } from 'shared/ReactSymbols'; import {readContext as readContextImpl} from './ReactFlightNewContext'; import {enableUseHook} from 'shared/ReactFeatureFlags'; -import { - getPreviouslyUsedThenableAtIndex, - createThenableState, - trackUsedThenable, -} from './ReactFlightThenable'; +import {createThenableState, trackUsedThenable} from './ReactFlightThenable'; let currentRequest = null; let thenableIndexCounter = 0; @@ -121,8 +117,6 @@ function useId(): string { return ':' + currentRequest.identifierPrefix + 'S' + id.toString(32) + ':'; } -function noop(): void {} - function use(usable: Usable): T { if (usable !== null && typeof usable === 'object') { // $FlowFixMe[method-unbinding] @@ -134,61 +128,10 @@ function use(usable: Usable): T { const index = thenableIndexCounter; thenableIndexCounter += 1; - switch (thenable.status) { - case 'fulfilled': { - const fulfilledValue: T = thenable.value; - return fulfilledValue; - } - case 'rejected': { - const rejectedError = thenable.reason; - throw rejectedError; - } - default: { - const prevThenableAtIndex: Thenable | null = getPreviouslyUsedThenableAtIndex( - thenableState, - index, - ); - if (prevThenableAtIndex !== null) { - if (thenable !== prevThenableAtIndex) { - // Avoid an unhandled rejection errors for the Promises that we'll - // intentionally ignore. - thenable.then(noop, noop); - } - switch (prevThenableAtIndex.status) { - case 'fulfilled': { - const fulfilledValue: T = prevThenableAtIndex.value; - return fulfilledValue; - } - case 'rejected': { - const rejectedError: mixed = prevThenableAtIndex.reason; - throw rejectedError; - } - default: { - // The thenable still hasn't resolved. Suspend with the same - // thenable as last time to avoid redundant listeners. - throw prevThenableAtIndex; - } - } - } else { - // This is the first time something has been used at this index. - // Stash the thenable at the current index so we can reuse it during - // the next attempt. - if (thenableState === null) { - thenableState = createThenableState(); - } - trackUsedThenable(thenableState, thenable, index); - - // Suspend. - // TODO: Throwing here is an implementation detail that allows us to - // unwind the call stack. But we shouldn't allow it to leak into - // userspace. Throw an opaque placeholder value instead of the - // actual thenable. If it doesn't get captured by the work loop, log - // a warning, because that means something in userspace must have - // caught it. - throw thenable; - } - } + if (thenableState === null) { + thenableState = createThenableState(); } + return trackUsedThenable(thenableState, thenable, index); } else if (usable.$$typeof === REACT_SERVER_CONTEXT_TYPE) { const context: ReactServerContext = (usable: any); return readContext(context); diff --git a/packages/react-server/src/ReactFlightThenable.js b/packages/react-server/src/ReactFlightThenable.js index eaeb635217d68..9a91587b04fd7 100644 --- a/packages/react-server/src/ReactFlightThenable.js +++ b/packages/react-server/src/ReactFlightThenable.js @@ -20,8 +20,7 @@ import type { RejectedThenable, } from 'shared/ReactTypes'; -// TODO: Sparse arrays are bad for performance. -export opaque type ThenableState = Array | void>; +export opaque type ThenableState = Array>; export function createThenableState(): ThenableState { // The ThenableState is created the first time a component suspends. If it @@ -29,12 +28,27 @@ export function createThenableState(): ThenableState { return []; } +function noop(): void {} + export function trackUsedThenable( thenableState: ThenableState, thenable: Thenable, index: number, -) { - thenableState[index] = thenable; +): T { + const previous = thenableState[index]; + if (previous === undefined) { + thenableState.push(thenable); + } else { + if (previous !== thenable) { + // Reuse the previous thenable, and drop the new one. We can assume + // they represent the same value, because components are idempotent. + + // Avoid an unhandled rejection errors for the Promises that we'll + // intentionally ignore. + thenable.then(noop, noop); + thenable = previous; + } + } // We use an expando to track the status and result of a thenable so that we // can synchronously unwrap the value. Think of this as an extension of the @@ -43,53 +57,48 @@ export function trackUsedThenable( // If the thenable doesn't have a status, set it to "pending" and attach // a listener that will update its status and result when it resolves. switch (thenable.status) { - case 'fulfilled': - case 'rejected': - // A thenable that already resolved shouldn't have been thrown, so this is - // unexpected. Suggests a mistake in a userspace data library. Don't track - // this thenable, because if we keep trying it will likely infinite loop - // without ever resolving. - // TODO: Log a warning? - break; + case 'fulfilled': { + const fulfilledValue: T = thenable.value; + return fulfilledValue; + } + case 'rejected': { + const rejectedError = thenable.reason; + throw rejectedError; + } default: { if (typeof thenable.status === 'string') { // Only instrument the thenable if the status if not defined. If // it's defined, but an unknown value, assume it's been instrumented by // some custom userspace implementation. We treat it as "pending". - break; + } else { + const pendingThenable: PendingThenable = (thenable: any); + pendingThenable.status = 'pending'; + pendingThenable.then( + fulfilledValue => { + if (thenable.status === 'pending') { + const fulfilledThenable: FulfilledThenable = (thenable: any); + fulfilledThenable.status = 'fulfilled'; + fulfilledThenable.value = fulfilledValue; + } + }, + (error: mixed) => { + if (thenable.status === 'pending') { + const rejectedThenable: RejectedThenable = (thenable: any); + rejectedThenable.status = 'rejected'; + rejectedThenable.reason = error; + } + }, + ); } - const pendingThenable: PendingThenable = (thenable: any); - pendingThenable.status = 'pending'; - pendingThenable.then( - fulfilledValue => { - if (thenable.status === 'pending') { - const fulfilledThenable: FulfilledThenable = (thenable: any); - fulfilledThenable.status = 'fulfilled'; - fulfilledThenable.value = fulfilledValue; - } - }, - (error: mixed) => { - if (thenable.status === 'pending') { - const rejectedThenable: RejectedThenable = (thenable: any); - rejectedThenable.status = 'rejected'; - rejectedThenable.reason = error; - } - }, - ); - break; - } - } -} -export function getPreviouslyUsedThenableAtIndex( - thenableState: ThenableState | null, - index: number, -): Thenable | null { - if (thenableState !== null) { - const thenable = thenableState[index]; - if (thenable !== undefined) { - return thenable; + // Suspend. + // TODO: Throwing here is an implementation detail that allows us to + // unwind the call stack. But we shouldn't allow it to leak into + // userspace. Throw an opaque placeholder value instead of the + // actual thenable. If it doesn't get captured by the work loop, log + // a warning, because that means something in userspace must have + // caught it. + throw thenable; } } - return null; }