Skip to content

Commit

Permalink
[Flight] Set Current Owner / Task When Calling console.error or invok…
Browse files Browse the repository at this point in the history
…ing onError/onPostpone (#30206)

Stacked on #30197.

This is similar to #30182 and #21610 in Fizz.

Track the current owner/stack/task on the task. This tracks it for
attribution when serializing child properties.

This lets us provide the right owner and createTask when we
console.error from inside Flight itself. This also affects the way we
print those logs on the client since we need the owner and stack. Now
console.errors that originate on the server gets the right stack on the
client:

<img width="760" alt="Screenshot 2024-07-03 at 6 03 13 PM"
src="https://github.com/facebook/react/assets/63648/913300f8-f364-4e66-a19d-362e8d776c64">

Unfortunately, because we don't track the stack we never pop it so it'll
keep tracking for serializing sibling properties. We rely on "children"
typically being the last property in the common case anyway. However,
this can lead to wrong attribution in some cases where the invalid
property is a next property (without a wrapping element) and there's a
previous element that doesn't. E.g. `<ClientComponent title={<div />}
invalid={nonSerializable} />` would use the div as the attribution
instead of ClientComponent.

I also wrap all of our own console.error, onError and onPostpone in the
context of the parent component. It's annoying to have to remember to do
this though.

We could always wrap the whole rendering in such as context but it would
add more overhead since this rarely actually happens. It might make
sense to track the whole current task instead to lower the overhead.
That's what we do in Fizz. We'd still have to remember to restore the
debug task though. I realize now Fizz doesn't do that neither so the
debug task isn't wrapping the console.errors that Fizz itself logs.
There's something off about that Flight and Fizz implementations don't
perfectly align.
  • Loading branch information
sebmarkbage authored Jul 4, 2024
1 parent 0b5835a commit f38c22b
Show file tree
Hide file tree
Showing 2 changed files with 287 additions and 110 deletions.
63 changes: 62 additions & 1 deletion packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2761,10 +2761,61 @@ describe('ReactFlight', () => {
);
});

// @gate __DEV__ && enableOwnerStacks
it('can get the component owner stacks for onError in dev', async () => {
const thrownError = new Error('hi');
let caughtError;
let ownerStack;

function Foo() {
return ReactServer.createElement(Bar, null);
}
function Bar() {
return ReactServer.createElement(
'div',
null,
ReactServer.createElement(Baz, null),
);
}
function Baz() {
throw thrownError;
}

ReactNoopFlightServer.render(
ReactServer.createElement(
'div',
null,
ReactServer.createElement(Foo, null),
),
{
onError(error, errorInfo) {
caughtError = error;
ownerStack = ReactServer.captureOwnerStack
? ReactServer.captureOwnerStack()
: null;
},
},
);

expect(caughtError).toBe(thrownError);
expect(normalizeCodeLocInfo(ownerStack)).toBe(
'\n in Bar (at **)' + '\n in Foo (at **)',
);
});

// @gate (enableOwnerStacks && enableServerComponentLogs) || !__DEV__
it('should not include component stacks in replayed logs (unless DevTools add them)', () => {
class MyError extends Error {
toJSON() {
return 123;
}
}

function Foo() {
return 'hi';
return ReactServer.createElement('div', null, [
'Womp womp: ',
new MyError('spaghetti'),
]);
}

function Bar() {
Expand All @@ -2781,11 +2832,18 @@ describe('ReactFlight', () => {
const transport = ReactNoopFlightServer.render(
ReactServer.createElement(App),
);

assertConsoleErrorDev([
'Each child in a list should have a unique "key" prop.' +
' See https://react.dev/link/warning-keys for more information.\n' +
' in Bar (at **)\n' +
' in App (at **)',
'Error objects cannot be rendered as text children. Try formatting it using toString().\n' +
' <div>Womp womp: {Error}</div>\n' +
' ^^^^^^^\n' +
' in Foo (at **)\n' +
' in Bar (at **)\n' +
' in App (at **)',
]);

// Replay logs on the client
Expand All @@ -2794,6 +2852,9 @@ describe('ReactFlight', () => {
[
'Each child in a list should have a unique "key" prop.' +
' See https://react.dev/link/warning-keys for more information.',
'Error objects cannot be rendered as text children. Try formatting it using toString().\n' +
' <div>Womp womp: {Error}</div>\n' +
' ^^^^^^^',
],
// We should not have a stack in the replay because that should be added either by console.createTask
// or React DevTools on the client. Neither of which we do here.
Expand Down
Loading

0 comments on commit f38c22b

Please sign in to comment.