Skip to content

Commit

Permalink
Use the Nearest Parent of an Errored Promise as its Owner (#29814)
Browse files Browse the repository at this point in the history
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):
<img width="813" alt="Screenshot 2024-06-06 at 5 16 20 PM"
src="https://github.com/facebook/react/assets/63648/7da2d4c9-539b-494e-ba63-1abdc58ff13c">

After (App):
<img width="811" alt="Screenshot 2024-06-08 at 12 29 23 AM"
src="https://github.com/facebook/react/assets/63648/dbf40bd7-c24d-4200-81a6-5018bef55f6d">
  • Loading branch information
sebmarkbage authored Jun 11, 2024
1 parent a26e3f4 commit 383b2a1
Show file tree
Hide file tree
Showing 7 changed files with 105 additions and 15 deletions.
2 changes: 1 addition & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}],
Expand Down
20 changes: 10 additions & 10 deletions packages/react-client/src/ReactFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> = {
Expand Down Expand Up @@ -1710,11 +1711,6 @@ function resolveHint<Code: HintCode>(
const supportsCreateTask =
__DEV__ && enableOwnerStacks && !!(console: any).createTask;

const taskCache: null | WeakMap<
ReactComponentInfo | ReactAsyncInfo,
ConsoleTask,
> = supportsCreateTask ? new WeakMap() : null;

type FakeFunction<T> = (() => T) => T;
const fakeFunctionCache: Map<string, FakeFunction<any>> = __DEV__
? new Map()
Expand Down Expand Up @@ -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;
}
Expand All @@ -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(
Expand Down
19 changes: 16 additions & 3 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
24 changes: 23 additions & 1 deletion packages/react-reconciler/src/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import {
enableRefAsProp,
enableAsyncIterableChildren,
disableLegacyMode,
enableOwnerStacks,
} from 'shared/ReactFeatureFlags';

import {
Expand Down Expand Up @@ -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 {
Expand Down
21 changes: 21 additions & 0 deletions packages/react-reconciler/src/getComponentNameFromFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down
33 changes: 33 additions & 0 deletions packages/react-server/src/ReactFlightServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ export type ReactClientValue =
// subtype, so the receiver can only accept once of these.
| React$Element<string>
| React$Element<ClientReference<any> & any>
| ReactComponentInfo
| string
| boolean
| number
Expand Down Expand Up @@ -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. ' +
Expand Down Expand Up @@ -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]);
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/shared/ReactTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ export type ReactComponentInfo = {
+env?: string,
+owner?: null | ReactComponentInfo,
+stack?: null | string,
+task?: null | ConsoleTask,
};

export type ReactAsyncInfo = {
Expand Down

0 comments on commit 383b2a1

Please sign in to comment.