From 383b2a18456215d2d3ec46f33c0c912e84efa08f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 11 Jun 2024 18:10:24 -0400 Subject: [PATCH] Use the Nearest Parent of an Errored Promise as its Owner (#29814) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on #29807. 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 `Error` 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. Before this stack the "owner" and "task" of a Lazy that errors was the nearest Fiber but if the thing erroring is a Server Component, we need to get that as the owner from the inner most part of debugInfo. To get the Task for that Server Component, we need to expose it on the ReactComponentInfo object. Unfortunately that makes the object not serializable so we need to special case this to exclude it from serialization. It gets restored again on the client. Before (Shell): Screenshot 2024-06-06 at 5 16 20 PM After (App): Screenshot 2024-06-08 at 12 29 23 AM --- .eslintrc.js | 2 +- .../react-client/src/ReactFlightClient.js | 20 +++++------ .../src/__tests__/ReactFlight-test.js | 19 +++++++++-- .../react-reconciler/src/ReactChildFiber.js | 24 +++++++++++++- .../src/getComponentNameFromFiber.js | 21 ++++++++++++ .../react-server/src/ReactFlightServer.js | 33 +++++++++++++++++++ packages/shared/ReactTypes.js | 1 + 7 files changed, 105 insertions(+), 15 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 01d91a150b..05a6ecae3a 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 1fec10eb0d..086a3ae46b 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 cef237fe4a..54d66bdac1 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-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 057b62a504..db8f0e6233 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,28 @@ 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') { + throwFiber._debugOwner = (debugInfo[i]: any); + if (enableOwnerStacks) { + throwFiber._debugTask = debugInfo[i].task; + } + break; + } + } + } } return throwFiber; } finally { diff --git a/packages/react-reconciler/src/getComponentNameFromFiber.js b/packages/react-reconciler/src/getComponentNameFromFiber.js index 332324900f..fbf5f708e2 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; diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index c5ea7ad376..670c11c7c5 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 e0140ac535..a108ead17d 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 = {