From f6c0d7a378ddc723e9386de72fe289690c67f7a2 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 13 Sep 2022 23:53:19 -0400 Subject: [PATCH] Treat blocked modules or models as a special status We currently loop over all chunks at the end to error them if they're still pending. We shouldn't do this if they're pending because they're blocked on an external resource like a module because the module might not resolve before the Flight connection closes and that's not an error. In an alternative solution I had a set that tracked pending chunks and removed one at a time. While the loop at the end is faster it's more work as we go. I figured the extra status might also help debugging. For modules we can probably assume no forward references, and the first async module we can just use the promise as the chunk. So we could probably get away with this only on models that are blocked by modules. --- .../react-client/src/ReactFlightClient.js | 59 ++++++++++++++----- .../src/ReactFiberWakeable.new.js | 9 ++- 2 files changed, 49 insertions(+), 19 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index dc8cd2e0ce1ba..3ca33ff00e3d3 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -38,6 +38,7 @@ export type JSONValue = | $ReadOnlyArray; const PENDING = 'pending'; +const BLOCKED = 'blocked'; const RESOLVED_MODEL = 'resolved_model'; const RESOLVED_MODULE = 'resolved_module'; const INITIALIZED = 'fulfilled'; @@ -50,6 +51,13 @@ type PendingChunk = { _response: Response, then(resolve: (T) => mixed, reject: (mixed) => mixed): void, }; +type BlockedChunk = { + status: 'blocked', + value: null | Array<(T) => mixed>, + reason: null | Array<(mixed) => mixed>, + _response: Response, + then(resolve: (T) => mixed, reject: (mixed) => mixed): void, +}; type ResolvedModelChunk = { status: 'resolved_model', value: UninitializedModel, @@ -80,6 +88,7 @@ type ErroredChunk = { }; type SomeChunk = | PendingChunk + | BlockedChunk | ResolvedModelChunk | ResolvedModuleChunk | InitializedChunk @@ -115,6 +124,7 @@ Chunk.prototype.then = function( resolve(chunk.value); break; case PENDING: + case BLOCKED: if (resolve) { if (chunk.value === null) { chunk.value = []; @@ -159,8 +169,9 @@ function readChunk(chunk: SomeChunk): T { case INITIALIZED: return chunk.value; case PENDING: + case BLOCKED: // eslint-disable-next-line no-throw-literal - throw (chunk: Thenable); + throw ((chunk: any): Thenable); default: throw chunk.reason; } @@ -177,6 +188,11 @@ function createPendingChunk(response: Response): PendingChunk { return new Chunk(PENDING, null, null, response); } +function createBlockedChunk(response: Response): BlockedChunk { + // $FlowFixMe Flow doesn't support functions as constructors + return new Chunk(BLOCKED, null, null, response); +} + function createErrorChunk( response: Response, error: Error, @@ -210,6 +226,7 @@ function wakeChunkIfInitialized( wakeChunk(resolveListeners, chunk.value); break; case PENDING: + case BLOCKED: chunk.value = resolveListeners; chunk.reason = rejectListeners; break; @@ -222,7 +239,7 @@ function wakeChunkIfInitialized( } function triggerErrorOnChunk(chunk: SomeChunk, error: mixed): void { - if (chunk.status !== PENDING) { + if (chunk.status !== PENDING && chunk.status !== BLOCKED) { // We already resolved. We didn't expect to see this. return; } @@ -278,7 +295,7 @@ function resolveModuleChunk( chunk: SomeChunk, value: ModuleReference, ): void { - if (chunk.status !== PENDING) { + if (chunk.status !== PENDING && chunk.status !== BLOCKED) { // We already resolved. We didn't expect to see this. return; } @@ -308,11 +325,11 @@ function initializeModelChunk(chunk: ResolvedModelChunk): void { ) { initializingChunkBlockedModel.value = value; // We discovered new dependencies on modules that are not yet resolved. - // We have to return to the PENDING state until they're resolved. - const pendingChunk: PendingChunk = (chunk: any); - pendingChunk.status = PENDING; - pendingChunk.value = null; - pendingChunk.reason = null; + // We have to go the BLOCKED state until they're resolved. + const blockedChunk: BlockedChunk = (chunk: any); + blockedChunk.status = BLOCKED; + blockedChunk.value = null; + blockedChunk.reason = null; } else { const initializedChunk: InitializedChunk = (chunk: any); initializedChunk.status = INITIALIZED; @@ -348,7 +365,9 @@ export function reportGlobalError(response: Response, error: Error): void { // If this chunk was already resolved or errored, it won't // trigger an error but if it wasn't then we need to // because we won't be getting any new data to resolve it. - triggerErrorOnChunk(chunk, error); + if (chunk.status === PENDING) { + triggerErrorOnChunk(chunk, error); + } }); } @@ -433,7 +452,7 @@ function createModelResolver( parentObject[key] = value; blocked.deps--; if (blocked.deps === 0) { - if (chunk.status !== PENDING) { + if (chunk.status !== BLOCKED) { return; } const resolveListeners = chunk.value; @@ -480,6 +499,7 @@ export function parseModelString( case INITIALIZED: return chunk.value; case PENDING: + case BLOCKED: const parentChunk = initializingChunk; chunk.then( createModelResolver(parentChunk, parentObject, key), @@ -573,21 +593,28 @@ export function resolveModule( // that we'll need them. const promise = preloadModule(moduleReference); if (promise) { - let pendingChunk; + let blockedChunk: BlockedChunk; if (!chunk) { - pendingChunk = createPendingChunk(response); - chunks.set(id, pendingChunk); + // Technically, we should just treat promise as the chunk in this + // case. Because it'll just behave as any other promise. + blockedChunk = createBlockedChunk(response); + chunks.set(id, blockedChunk); } else { - pendingChunk = chunk; + // This can't actually happen because we don't have any forward + // references to modules. + blockedChunk = (chunk: any); + blockedChunk.status = BLOCKED; } promise.then( - () => resolveModuleChunk(pendingChunk, moduleReference), - error => triggerErrorOnChunk(pendingChunk, error), + () => resolveModuleChunk(blockedChunk, moduleReference), + error => triggerErrorOnChunk(blockedChunk, error), ); } else { if (!chunk) { chunks.set(id, createResolvedModuleChunk(response, moduleReference)); } else { + // This can't actually happen because we don't have any forward + // references to modules. resolveModuleChunk(chunk, moduleReference); } } diff --git a/packages/react-reconciler/src/ReactFiberWakeable.new.js b/packages/react-reconciler/src/ReactFiberWakeable.new.js index 11f470cfab888..96a3dd9745884 100644 --- a/packages/react-reconciler/src/ReactFiberWakeable.new.js +++ b/packages/react-reconciler/src/ReactFiberWakeable.new.js @@ -75,9 +75,12 @@ export function trackSuspendedWakeable(wakeable: Wakeable) { suspendedThenable = null; break; default: { - // TODO: 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. + 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. + break; + } const pendingThenable: PendingThenable = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then(