From 5d4603f8fc5f1dcd2f4678bc909a6dc2a36105cd Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 7 Jun 2024 21:16:59 -0400 Subject: [PATCH 1/4] Stash the task for each Server Component If this is happening on the server where the FlightClient generates objects that will then be consumed again by the Flight server, we need to strip the task out before serializing it since it's not serializable but it can be restored from stack + owner. --- .eslintrc.js | 2 +- .../react-client/src/ReactFlightClient.js | 20 +++++------ .../src/__tests__/ReactFlight-test.js | 19 +++++++++-- .../react-server/src/ReactFlightServer.js | 33 +++++++++++++++++++ packages/shared/ReactTypes.js | 1 + 5 files changed, 61 insertions(+), 14 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 01d91a150bce1..05a6ecae3ade8 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -245,7 +245,7 @@ module.exports = { }, ], 'no-shadow': ERROR, - 'no-unused-vars': [ERROR, {args: 'none'}], + 'no-unused-vars': [ERROR, {args: 'none', ignoreRestSiblings: true}], 'no-use-before-define': OFF, 'no-useless-concat': OFF, quotes: [ERROR, 'single', {avoidEscape: true, allowTemplateLiterals: true}], diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 1fec10eb0d86c..086a3ae46b802 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -162,6 +162,7 @@ type InitializedStreamChunk< value: T, reason: FlightStreamController, _response: Response, + _debugInfo?: null | ReactDebugInfo, then(resolve: (ReadableStream) => mixed, reject?: (mixed) => mixed): void, }; type ErroredChunk = { @@ -1710,11 +1711,6 @@ function resolveHint( const supportsCreateTask = __DEV__ && enableOwnerStacks && !!(console: any).createTask; -const taskCache: null | WeakMap< - ReactComponentInfo | ReactAsyncInfo, - ConsoleTask, -> = supportsCreateTask ? new WeakMap() : null; - type FakeFunction = (() => T) => T; const fakeFunctionCache: Map> = __DEV__ ? new Map() @@ -1834,12 +1830,12 @@ function initializeFakeTask( response: Response, debugInfo: ReactComponentInfo | ReactAsyncInfo, ): null | ConsoleTask { - if (taskCache === null || typeof debugInfo.stack !== 'string') { + if (!supportsCreateTask || typeof debugInfo.stack !== 'string') { return null; } const componentInfo: ReactComponentInfo = (debugInfo: any); // Refined const stack: string = debugInfo.stack; - const cachedEntry = taskCache.get((componentInfo: any)); + const cachedEntry = componentInfo.task; if (cachedEntry !== undefined) { return cachedEntry; } @@ -1856,16 +1852,20 @@ function initializeFakeTask( ); const callStack = buildFakeCallStack(response, stack, createTaskFn); + let componentTask; if (ownerTask === null) { const rootTask = response._debugRootTask; if (rootTask != null) { - return rootTask.run(callStack); + componentTask = rootTask.run(callStack); } else { - return callStack(); + componentTask = callStack(); } } else { - return ownerTask.run(callStack); + componentTask = ownerTask.run(callStack); } + // $FlowFixMe[cannot-write]: We consider this part of initialization. + componentInfo.task = componentTask; + return componentTask; } function resolveDebugInfo( diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index cef237fe4aa9f..54d66bdac10d0 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -27,14 +27,27 @@ function normalizeCodeLocInfo(str) { ); } +function normalizeComponentInfo(debugInfo) { + if (typeof debugInfo.stack === 'string') { + const {task, ...copy} = debugInfo; + copy.stack = normalizeCodeLocInfo(debugInfo.stack); + if (debugInfo.owner) { + copy.owner = normalizeComponentInfo(debugInfo.owner); + } + return copy; + } else { + return debugInfo; + } +} + function getDebugInfo(obj) { const debugInfo = obj._debugInfo; if (debugInfo) { + const copy = []; for (let i = 0; i < debugInfo.length; i++) { - if (typeof debugInfo[i].stack === 'string') { - debugInfo[i].stack = normalizeCodeLocInfo(debugInfo[i].stack); - } + copy.push(normalizeComponentInfo(debugInfo[i])); } + return copy; } return debugInfo; } diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index c5ea7ad376ad5..670c11c7c53cf 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -352,6 +352,7 @@ export type ReactClientValue = // subtype, so the receiver can only accept once of these. | React$Element | React$Element & any> + | ReactComponentInfo | string | boolean | number @@ -2462,6 +2463,32 @@ function renderModelDestructive( ); } if (__DEV__) { + if ( + // TODO: We don't currently have a brand check on ReactComponentInfo. Reconsider. + typeof value.task === 'object' && + value.task !== null && + // $FlowFixMe[method-unbinding] + typeof value.task.run === 'function' && + typeof value.name === 'string' && + typeof value.env === 'string' && + value.owner !== undefined && + (enableOwnerStacks + ? typeof (value: any).stack === 'string' + : typeof (value: any).stack === 'undefined') + ) { + // This looks like a ReactComponentInfo. We can't serialize the ConsoleTask object so we + // need to omit it before serializing. + const componentDebugInfo = { + name: value.name, + env: value.env, + owner: value.owner, + }; + if (enableOwnerStacks) { + (componentDebugInfo: any).stack = (value: any).stack; + } + return (componentDebugInfo: any); + } + if (objectName(value) !== 'Object') { console.error( 'Only plain objects can be passed to Client Components from Server Components. ' + @@ -3231,6 +3258,12 @@ function forwardDebugInfo( ) { for (let i = 0; i < debugInfo.length; i++) { request.pendingChunks++; + if (typeof debugInfo[i].name === 'string') { + // We outline this model eagerly so that we can refer to by reference as an owner. + // If we had a smarter way to dedupe we might not have to do this if there ends up + // being no references to this as an owner. + outlineModel(request, debugInfo[i]); + } emitDebugChunk(request, id, debugInfo[i]); } } diff --git a/packages/shared/ReactTypes.js b/packages/shared/ReactTypes.js index e0140ac53533a..a108ead17d624 100644 --- a/packages/shared/ReactTypes.js +++ b/packages/shared/ReactTypes.js @@ -183,6 +183,7 @@ export type ReactComponentInfo = { +env?: string, +owner?: null | ReactComponentInfo, +stack?: null | string, + +task?: null | ConsoleTask, }; export type ReactAsyncInfo = { From b4750d260f56ad05ef19223d1b552aee161b6438 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 7 Jun 2024 23:29:50 -0400 Subject: [PATCH 2/4] Use the parent as the owner and task Whether that nearest parent is a Server Component or the nearest Fiber. --- .../react-reconciler/src/ReactChildFiber.js | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 057b62a504b31..c09405b2060e8 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -48,6 +48,7 @@ import { enableRefAsProp, enableAsyncIterableChildren, disableLegacyMode, + enableOwnerStacks, } from 'shared/ReactFeatureFlags'; import { @@ -1959,7 +1960,30 @@ function createChildReconciler( const throwFiber = createFiberFromThrow(x, returnFiber.mode, lanes); throwFiber.return = returnFiber; if (__DEV__) { - throwFiber._debugInfo = currentDebugInfo; + const debugInfo = (throwFiber._debugInfo = currentDebugInfo); + // Conceptually the error's owner/task should ideally be captured when the + // Error constructor is called but neither console.createTask does this, + // nor do we override them to capture our `owner`. So instead, we use the + // nearest parent as the owner/task of the error. This is usually the same + // thing when it's thrown from the same async component but not if you await + // a promise started from a different component/task. + throwFiber._debugOwner = returnFiber._debugOwner; + if (enableOwnerStacks) { + throwFiber._debugTask = returnFiber._debugTask; + } + if (debugInfo != null) { + for (let i = debugInfo.length - 1; i >= 0; i--) { + if (typeof debugInfo[i].stack === 'string') { + // Ideally we could get the Task from this object but we don't expose it + // from Flight since it's in a WeakMap so we use the + throwFiber._debugOwner = (debugInfo[i]: any); + if (enableOwnerStacks) { + throwFiber._debugTask = debugInfo[i].task; + } + break; + } + } + } } return throwFiber; } finally { From d45bdd0d27665450a91a16e8c64681728b09072d Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sat, 8 Jun 2024 00:13:55 -0400 Subject: [PATCH 3/4] Use the name of the inner most parent for messages --- .../src/getComponentNameFromFiber.js | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/react-reconciler/src/getComponentNameFromFiber.js b/packages/react-reconciler/src/getComponentNameFromFiber.js index 332324900fa88..fbf5f708e2340 100644 --- a/packages/react-reconciler/src/getComponentNameFromFiber.js +++ b/packages/react-reconciler/src/getComponentNameFromFiber.js @@ -44,6 +44,7 @@ import { LegacyHiddenComponent, CacheComponent, TracingMarkerComponent, + Throw, } from 'react-reconciler/src/ReactWorkTags'; import getComponentNameFromType from 'shared/getComponentNameFromType'; import {REACT_STRICT_MODE_TYPE} from 'shared/ReactSymbols'; @@ -160,6 +161,26 @@ export default function getComponentNameFromFiber(fiber: Fiber): string | null { if (enableLegacyHidden) { return 'LegacyHidden'; } + break; + case Throw: { + if (__DEV__) { + // For an error in child position we use the of the inner most parent component. + // Whether a Server Component or the parent Fiber. + const debugInfo = fiber._debugInfo; + if (debugInfo != null) { + for (let i = debugInfo.length - 1; i >= 0; i--) { + if (typeof debugInfo[i].name === 'string') { + return debugInfo[i].name; + } + } + } + if (fiber.return === null) { + return null; + } + return getComponentNameFromFiber(fiber.return); + } + return null; + } } return null; From adc2f78b9d7dbcf658f6cef1f2b5c9043f920bc3 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 11 Jun 2024 15:44:23 -0400 Subject: [PATCH 4/4] Remove outdated comment --- packages/react-reconciler/src/ReactChildFiber.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index c09405b2060e8..db8f0e6233644 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -1974,8 +1974,6 @@ function createChildReconciler( if (debugInfo != null) { for (let i = debugInfo.length - 1; i >= 0; i--) { if (typeof debugInfo[i].stack === 'string') { - // Ideally we could get the Task from this object but we don't expose it - // from Flight since it's in a WeakMap so we use the throwFiber._debugOwner = (debugInfo[i]: any); if (enableOwnerStacks) { throwFiber._debugTask = debugInfo[i].task;