Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Flight] Implement error digests for Flight runtime and expose errorInfo in getDerivedStateFromError #25302

Merged
merged 7 commits into from
Sep 23, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 43 additions & 4 deletions packages/react-client/src/ReactFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -628,21 +628,60 @@ export function resolveSymbol(
chunks.set(id, createInitializedChunk(response, Symbol.for(name)));
}

export function resolveError(
type ErrorWithDigest = Error & {_digest?: string};
export function resolveErrorProd(
response: Response,
id: number,
digest: string,
): void {
if (__DEV__) {
// These errors should never make it into a build so we don't need to encode them in codes.json
// eslint-disable-next-line react-internal/prod-error-codes
throw new Error(
'resolveErrorProd should never be called in development mode. Use resolveErrorDev instead. This is a bug in React.',
);
}
const error = new Error('An error occurred in the Server Components render.');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the verbiage we want here for prod errors but since this is a generic error resolver we either need to pass in callsite specific prod-ready error messages or use something generic like this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth saying something about that it's intentionally excluded in prod. Ideally linking to a docs page explaining how to fix it.

error.stack = '';
(error: any)._digest = digest;
const errorWithDigest: ErrorWithDigest = (error: any);
const chunks = response._chunks;
const chunk = chunks.get(id);
if (!chunk) {
chunks.set(id, createErrorChunk(response, errorWithDigest));
} else {
triggerErrorOnChunk(chunk, errorWithDigest);
}
}

export function resolveErrorDev(
response: Response,
id: number,
digest: string,
message: string,
stack: string,
): void {
if (!__DEV__) {
// These errors should never make it into a build so we don't need to encode them in codes.json
// eslint-disable-next-line react-internal/prod-error-codes
throw new Error(
'resolveErrorDev should never be called in production mode. Use resolveErrorProd instead. This is a bug in React.',
);
}
// eslint-disable-next-line react-internal/prod-error-codes
const error = new Error(message);
const error = new Error(
message ||
'An error occurred in the Server Components render but no message was provided',
);
error.stack = stack;
(error: any)._digest = digest;
const errorWithDigest: ErrorWithDigest = (error: any);
const chunks = response._chunks;
const chunk = chunks.get(id);
if (!chunk) {
chunks.set(id, createErrorChunk(response, error));
chunks.set(id, createErrorChunk(response, errorWithDigest));
} else {
triggerErrorOnChunk(chunk, error);
triggerErrorOnChunk(chunk, errorWithDigest);
}
}

Expand Down
15 changes: 13 additions & 2 deletions packages/react-client/src/ReactFlightClientStream.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ import {
resolveModel,
resolveProvider,
resolveSymbol,
resolveError,
resolveErrorProd,
resolveErrorDev,
createResponse as createResponseBase,
parseModelString,
parseModelTuple,
Expand Down Expand Up @@ -62,7 +63,17 @@ function processFullRow(response: Response, row: string): void {
}
case 'E': {
const errorInfo = JSON.parse(text);
resolveError(response, id, errorInfo.message, errorInfo.stack);
if (__DEV__) {
resolveErrorDev(
response,
id,
errorInfo.digest,
errorInfo.message,
errorInfo.stack,
);
} else {
resolveErrorProd(response, id, errorInfo.digest);
}
return;
}
default: {
Expand Down
18 changes: 15 additions & 3 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,19 @@ describe('ReactFlight', () => {
componentDidMount() {
expect(this.state.hasError).toBe(true);
expect(this.state.error).toBeTruthy();
expect(this.state.error.message).toContain(this.props.expectedMessage);
if (__DEV__) {
expect(this.state.error.message).toContain(
this.props.expectedMessage,
);
expect(this.state.error._digest).toBe('a dev digest');
} else {
expect(this.state.error.message).toBe(
'An error occurred in the Server Components render.',
);
expect(this.state.error._digest).toContain(
this.props.expectedMessage,
);
}
}
render() {
if (this.state.hasError) {
Expand Down Expand Up @@ -371,8 +383,8 @@ describe('ReactFlight', () => {
}

const options = {
onError() {
// ignore
onError(x) {
return __DEV__ ? 'a dev digest' : `digest("${x.message}")`;
},
};
const event = ReactNoopFlightServer.render(<EventHandlerProp />, options);
Expand Down
20 changes: 17 additions & 3 deletions packages/react-server-dom-relay/src/ReactFlightDOMRelayClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ import {
resolveModel,
resolveModule,
resolveSymbol,
resolveError,
resolveErrorDev,
resolveErrorProd,
close,
getRoot,
} from 'react-client/src/ReactFlightClient';
Expand All @@ -34,7 +35,20 @@ export function resolveRow(response: Response, chunk: RowEncoding): void {
// $FlowFixMe: Flow doesn't support disjoint unions on tuples.
resolveSymbol(response, chunk[1], chunk[2]);
} else {
// $FlowFixMe: Flow doesn't support disjoint unions on tuples.
resolveError(response, chunk[1], chunk[2].message, chunk[2].stack);
if (__DEV__) {
resolveErrorDev(
response,
chunk[1],
// $FlowFixMe: Flow doesn't support disjoint unions on tuples.
chunk[2].digest,
// $FlowFixMe: Flow doesn't support disjoint unions on tuples.
chunk[2].message || '',
// $FlowFixMe: Flow doesn't support disjoint unions on tuples.
chunk[2].stack || '',
);
} else {
// $FlowFixMe: Flow doesn't support disjoint unions on tuples.
resolveErrorProd(response, chunk[1], chunk[2].digest);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ export type RowEncoding =
'E',
number,
{
message: string,
stack: string,
digest: string,
message?: string,
stack?: string,
...
},
];
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,48 @@ export function resolveModuleMetaData<T>(

export type Chunk = RowEncoding;

export function processErrorChunk(
export function processErrorChunkProd(
request: Request,
id: number,
digest: string,
): Chunk {
if (__DEV__) {
// These errors should never make it into a build so we don't need to encode them in codes.json
// eslint-disable-next-line react-internal/prod-error-codes
throw new Error(
'processErrorChunkProd should never be called while in development mode. Use processErrorChunkDev instead. This is a bug in React.',
);
}

return [
'E',
id,
{
digest,
},
];
}

export function processErrorChunkDev(
request: Request,
id: number,
digest: string,
message: string,
stack: string,
): Chunk {
if (!__DEV__) {
// These errors should never make it into a build so we don't need to encode them in codes.json
// eslint-disable-next-line react-internal/prod-error-codes
throw new Error(
'processErrorChunkDev should never be called while in production mode. Use processErrorChunkProd instead. This is a bug in React.',
);
}

return [
'E',
id,
{
digest,
message,
stack,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,13 @@ describe('ReactFlightDOM', () => {

function MyErrorBoundary({children}) {
return (
<ErrorBoundary fallback={e => <p>{e.message}</p>}>
<ErrorBoundary
fallback={e => (
<p>
{__DEV__ ? e.message + ' + ' : null}
{e._digest}
</p>
)}>
{children}
</ErrorBoundary>
);
Expand Down Expand Up @@ -434,6 +440,7 @@ describe('ReactFlightDOM', () => {
{
onError(x) {
reportedErrors.push(x);
return __DEV__ ? 'a dev digest' : `digest("${x.message}")`;
},
},
);
Expand Down Expand Up @@ -477,11 +484,14 @@ describe('ReactFlightDOM', () => {
await act(async () => {
rejectGames(theError);
});
const expectedGamesValue = __DEV__
? '<p>Game over + a dev digest</p>'
: '<p>digest("Game over")</p>';
expect(container.innerHTML).toBe(
'<div>:name::avatar:</div>' +
'<p>(loading sidebar)</p>' +
'<p>(loading posts)</p>' +
'<p>Game over</p>', // TODO: should not have message in prod.
expectedGamesValue,
);

expect(reportedErrors).toEqual([theError]);
Expand All @@ -495,7 +505,7 @@ describe('ReactFlightDOM', () => {
'<div>:name::avatar:</div>' +
'<div>:photos::friends:</div>' +
'<p>(loading posts)</p>' +
'<p>Game over</p>', // TODO: should not have message in prod.
expectedGamesValue,
);

// Show everything.
Expand All @@ -506,7 +516,7 @@ describe('ReactFlightDOM', () => {
'<div>:name::avatar:</div>' +
'<div>:photos::friends:</div>' +
'<div>:posts:</div>' +
'<p>Game over</p>', // TODO: should not have message in prod.
expectedGamesValue,
);

expect(reportedErrors).toEqual([]);
Expand Down Expand Up @@ -611,6 +621,8 @@ describe('ReactFlightDOM', () => {
{
onError(x) {
reportedErrors.push(x);
const message = typeof x === 'string' ? x : x.message;
return __DEV__ ? 'a dev digest' : `digest("${message}")`;
},
},
);
Expand All @@ -626,7 +638,13 @@ describe('ReactFlightDOM', () => {

await act(async () => {
root.render(
<ErrorBoundary fallback={e => <p>{e.message}</p>}>
<ErrorBoundary
fallback={e => (
<p>
{__DEV__ ? e.message + ' + ' : null}
{e._digest}
</p>
)}>
<Suspense fallback={<p>(loading)</p>}>
<App res={response} />
</Suspense>
Expand All @@ -638,7 +656,13 @@ describe('ReactFlightDOM', () => {
await act(async () => {
abort('for reasons');
});
expect(container.innerHTML).toBe('<p>Error: for reasons</p>');
if (__DEV__) {
expect(container.innerHTML).toBe(
'<p>Error: for reasons + a dev digest</p>',
);
} else {
expect(container.innerHTML).toBe('<p>digest("for reasons")</p>');
}

expect(reportedErrors).toEqual(['for reasons']);
});
Expand Down Expand Up @@ -772,7 +796,8 @@ describe('ReactFlightDOM', () => {
webpackMap,
{
onError(x) {
reportedErrors.push(x);
reportedErrors.push(x.message);
return __DEV__ ? 'a dev digest' : `digest("${x.message}")`;
},
},
);
Expand All @@ -789,15 +814,27 @@ describe('ReactFlightDOM', () => {

await act(async () => {
root.render(
<ErrorBoundary fallback={e => <p>{e.message}</p>}>
<ErrorBoundary
fallback={e => (
<p>
{__DEV__ ? e.message + ' + ' : null}
{e._digest}
</p>
)}>
<Suspense fallback={<p>(loading)</p>}>
<App res={response} />
</Suspense>
</ErrorBoundary>,
);
});
expect(container.innerHTML).toBe('<p>bug in the bundler</p>');
if (__DEV__) {
expect(container.innerHTML).toBe(
'<p>bug in the bundler + a dev digest</p>',
);
} else {
expect(container.innerHTML).toBe('<p>digest("bug in the bundler")</p>');
}

expect(reportedErrors).toEqual([]);
expect(reportedErrors).toEqual(['bug in the bundler']);
});
});
Loading