Skip to content

Commit

Permalink
Include server component names in the componentStack in DEV (#28415)
Browse files Browse the repository at this point in the history
I'm a bit ambivalent about this one because it's not the main strategy
that I plan on pursuing. I plan on replacing most DEV-only specific
stacks like `console.error` stacks with a new take on owner stacks and
native stacks. The future owner stacks may or may not be exposed to
error boundaries in DEV but if they are they'd be a new errorInfo
property since they're owner based and not available in prod.

The use case in `console.error` mostly goes away in the future so this
PR is mainly for error boundaries. It doesn't hurt to have it in there
while I'm working on the better stacks though.

The `componentStack` property exposed to error boundaries is more like
production behavior similar to `new Error().stack` (which even in DEV
won't ever expose owner stacks because `console.createTask` doesn't
affect these). I'm not sure it's worth adding server components in DEV
(this PR) because then you have forked behavior between dev and prod.

However, since even in the future there won't be any other place to get
the *parent* stack, maybe this can be useful information even if it's
only dev. We could expose a third property on errorInfo that's DEV only
and parent stack but including server components. That doesn't seem
worth it over just having the stack differ in dev and prod.

I don't plan on adding line/column number to these particular stacks.

A follow up could be to add this to Fizz prerender too but only in DEV.
  • Loading branch information
sebmarkbage committed Feb 23, 2024
1 parent 66c8346 commit 8fb0233
Show file tree
Hide file tree
Showing 6 changed files with 177 additions and 0 deletions.
102 changes: 102 additions & 0 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@

'use strict';

function normalizeCodeLocInfo(str) {
return (
str &&
str.replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function (m, name) {
return '\n in ' + name + (/\d/.test(m) ? ' (at **)' : '');
})
);
}

const heldValues = [];
let finalizationCallback;
function FinalizationRegistryMock(callback) {
Expand Down Expand Up @@ -69,6 +78,14 @@ describe('ReactFlight', () => {
error,
};
}
componentDidCatch(error, errorInfo) {
expect(error).toBe(this.state.error);
if (this.props.expectedStack !== undefined) {
expect(normalizeCodeLocInfo(errorInfo.componentStack)).toBe(
this.props.expectedStack,
);
}
}
componentDidMount() {
expect(this.state.hasError).toBe(true);
expect(this.state.error).toBeTruthy();
Expand Down Expand Up @@ -900,6 +917,91 @@ describe('ReactFlight', () => {
});
});

it('should include server components in error boundary stacks in dev', async () => {
const ClientErrorBoundary = clientReference(ErrorBoundary);

function Throw({value}) {
throw value;
}

const expectedStack = __DEV__
? // TODO: This should include Throw but it doesn't have a Fiber.
'\n in div' + '\n in ErrorBoundary (at **)' + '\n in App'
: '\n in div' + '\n in ErrorBoundary (at **)';

function App() {
return (
<ClientErrorBoundary
expectedMessage="This is a real Error."
expectedStack={expectedStack}>
<div>
<Throw value={new TypeError('This is a real Error.')} />
</div>
</ClientErrorBoundary>
);
}

const transport = ReactNoopFlightServer.render(<App />, {
onError(x) {
if (__DEV__) {
return 'a dev digest';
}
if (x instanceof Error) {
return `digest("${x.message}")`;
} else if (Array.isArray(x)) {
return `digest([])`;
} else if (typeof x === 'object' && x !== null) {
return `digest({})`;
}
return `digest(${String(x)})`;
},
});

await act(() => {
startTransition(() => {
ReactNoop.render(ReactNoopFlightClient.read(transport));
});
});
});

it('should include server components in warning stacks', async () => {
function Component() {
// Trigger key warning
return <div>{[<span />]}</div>;
}
const ClientComponent = clientReference(Component);

function Indirection({children}) {
return children;
}

function App() {
return (
<Indirection>
<ClientComponent />
</Indirection>
);
}

const transport = ReactNoopFlightServer.render(<App />);

await expect(async () => {
await act(() => {
startTransition(() => {
ReactNoop.render(ReactNoopFlightClient.read(transport));
});
});
}).toErrorDev(
'Each child in a list should have a unique "key" prop.\n' +
'\n' +
'Check the render method of `Component`. See https://reactjs.org/link/warning-keys for more information.\n' +
' in span (at **)\n' +
' in Component (at **)\n' +
' in Indirection (at **)\n' +
' in App (at **)',
);
});

it('should trigger the inner most error boundary inside a Client Component', async () => {
function ServerComponent() {
throw new Error('This was thrown in the Server Component.');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,41 @@ describe('component stack', () => {
'\n in Example (at **)',
);
});

// @reactVersion >=18.3
it('should log the current component stack with debug info from promises', () => {
const Child = () => {
console.error('Test error.');
console.warn('Test warning.');
return null;
};
const ChildPromise = Promise.resolve(<Child />);
ChildPromise.status = 'fulfilled';
ChildPromise.value = <Child />;
ChildPromise._debugInfo = [
{
name: 'ServerComponent',
env: 'Server',
},
];
const Parent = () => ChildPromise;
const Grandparent = () => <Parent />;

act(() => render(<Grandparent />));

expect(mockError).toHaveBeenCalledWith(
'Test error.',
'\n in Child (at **)' +
'\n in ServerComponent (at **)' +
'\n in Parent (at **)' +
'\n in Grandparent (at **)',
);
expect(mockWarn).toHaveBeenCalledWith(
'Test warning.',
'\n in Child (at **)' +
'\n in ServerComponent (at **)' +
'\n in Parent (at **)' +
'\n in Grandparent (at **)',
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ export function describeBuiltInComponentFrame(
return '\n' + prefix + name;
}

export function describeDebugInfoFrame(name: string, env: ?string): string {
return describeBuiltInComponentFrame(
name + (env ? ' (' + env + ')' : ''),
null,
);
}

let reentry = false;
let componentFrameCache;
if (__DEV__) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
describeBuiltInComponentFrame,
describeFunctionComponentFrame,
describeClassComponentFrame,
describeDebugInfoFrame,
} from './DevToolsComponentStackFrame';

export function describeFiber(
Expand Down Expand Up @@ -87,6 +88,16 @@ export function getStackByFiberInDevAndProd(
let node: Fiber = workInProgress;
do {
info += describeFiber(workTagMap, node, currentDispatcherRef);
// Add any Server Component stack frames in reverse order.
const debugInfo = node._debugInfo;
if (debugInfo) {
for (let i = debugInfo.length - 1; i >= 0; i--) {
const entry = debugInfo[i];
if (typeof entry.name === 'string') {
info += describeDebugInfoFrame(entry.name, entry.env);
}
}
}
// $FlowFixMe[incompatible-type] we bail out when we get a null
node = node.return;
} while (node);
Expand Down
13 changes: 13 additions & 0 deletions packages/react-reconciler/src/ReactFiberComponentStack.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
describeBuiltInComponentFrame,
describeFunctionComponentFrame,
describeClassComponentFrame,
describeDebugInfoFrame,
} from 'shared/ReactComponentStackFrame';

function describeFiber(fiber: Fiber): string {
Expand Down Expand Up @@ -64,6 +65,18 @@ export function getStackByFiberInDevAndProd(workInProgress: Fiber): string {
let node: Fiber = workInProgress;
do {
info += describeFiber(node);
if (__DEV__) {
// Add any Server Component stack frames in reverse order.
const debugInfo = node._debugInfo;
if (debugInfo) {
for (let i = debugInfo.length - 1; i >= 0; i--) {
const entry = debugInfo[i];
if (typeof entry.name === 'string') {
info += describeDebugInfoFrame(entry.name, entry.env);
}
}
}
}
// $FlowFixMe[incompatible-type] we bail out when we get a null
node = node.return;
} while (node);
Expand Down
7 changes: 7 additions & 0 deletions packages/shared/ReactComponentStackFrame.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ export function describeBuiltInComponentFrame(
}
}

export function describeDebugInfoFrame(name: string, env: ?string): string {
return describeBuiltInComponentFrame(
name + (env ? ' (' + env + ')' : ''),
null,
);
}

let reentry = false;
let componentFrameCache;
if (__DEV__) {
Expand Down

0 comments on commit 8fb0233

Please sign in to comment.