From 4a3b5eb61f1db0846ac89bf759a55b84a8bb2980 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 1 Jul 2024 10:40:56 -0400 Subject: [PATCH] Implement onError signature for renderToMarkup This way we can get parent and owner stacks from the error. This forces us to confront multiple errors and whether or not a Flight error that ends up being unobservable needs to really reject the render. This implements stashing of Flight errors with a digest and only errors if they end up erroring the Fizz render too. At this point they'll have parent stacks so we can surface those. --- packages/react-html/src/ReactHTMLClient.js | 11 +++- packages/react-html/src/ReactHTMLServer.js | 35 ++++++++++- .../src/__tests__/ReactHTMLClient-test.js | 62 +++++++++++++++++++ .../src/__tests__/ReactHTMLServer-test.js | 58 +++++++++++++++++ 4 files changed, 161 insertions(+), 5 deletions(-) diff --git a/packages/react-html/src/ReactHTMLClient.js b/packages/react-html/src/ReactHTMLClient.js index 533ae7a3c3e7b..e12b76356ef02 100644 --- a/packages/react-html/src/ReactHTMLClient.js +++ b/packages/react-html/src/ReactHTMLClient.js @@ -8,6 +8,7 @@ */ import type {ReactNodeList} from 'shared/ReactTypes'; +import type {ErrorInfo} from 'react-server/src/ReactFizzServer'; import ReactVersion from 'shared/ReactVersion'; @@ -27,6 +28,7 @@ import { type MarkupOptions = { identifierPrefix?: string, signal?: AbortSignal, + onError?: (error: mixed, errorInfo: ErrorInfo) => ?string, }; export function renderToMarkup( @@ -49,11 +51,16 @@ export function renderToMarkup( reject(error); }, }; - function onError(error: mixed) { + function handleError(error: mixed, errorInfo: ErrorInfo) { // Any error rejects the promise, regardless of where it happened. // Unlike other React SSR we don't want to put Suspense boundaries into // client rendering mode because there's no client rendering here. reject(error); + + const onError = options && options.onError; + if (onError) { + onError(error, errorInfo); + } } const resumableState = createResumableState( options ? options.identifierPrefix : undefined, @@ -72,7 +79,7 @@ export function renderToMarkup( ), createRootFormatContext(), Infinity, - onError, + handleError, undefined, undefined, undefined, diff --git a/packages/react-html/src/ReactHTMLServer.js b/packages/react-html/src/ReactHTMLServer.js index 923881e4da755..2b87d8eeea448 100644 --- a/packages/react-html/src/ReactHTMLServer.js +++ b/packages/react-html/src/ReactHTMLServer.js @@ -9,6 +9,7 @@ import type {ReactNodeList} from 'shared/ReactTypes'; import type {LazyComponent} from 'react/src/ReactLazy'; +import type {ErrorInfo} from 'react-server/src/ReactFizzServer'; import ReactVersion from 'shared/ReactVersion'; @@ -62,6 +63,7 @@ type ReactMarkupNodeList = type MarkupOptions = { identifierPrefix?: string, signal?: AbortSignal, + onError?: (error: mixed, errorInfo: ErrorInfo) => ?string, }; function noServerCallOrFormAction() { @@ -109,17 +111,44 @@ export function renderToMarkup( reject(error); }, }; - function onError(error: mixed) { + + let stashErrorIdx = 1; + const stashedErrors: Map = new Map(); + + function handleFlightError(error: mixed): string { + // For Flight errors we don't immediately reject, because they might not matter + // to the output of the HTML. We stash the error with a digest in case we need + // to get to the original error from the Fizz render. + const id = '' + stashErrorIdx++; + stashedErrors.set(id, error); + return id; + } + + function handleError(error: mixed, errorInfo: ErrorInfo) { + if (typeof error === 'object' && error !== null) { + const id = error.digest; + // Note that the original error might be `undefined` so we need a has check. + if (typeof id === 'string' && stashedErrors.has(id)) { + // Get the original error thrown inside Flight. + error = stashedErrors.get(id); + } + } + // Any error rejects the promise, regardless of where it happened. // Unlike other React SSR we don't want to put Suspense boundaries into // client rendering mode because there's no client rendering here. reject(error); + + const onError = options && options.onError; + if (onError) { + onError(error, errorInfo); + } } const flightRequest = createFlightRequest( // $FlowFixMe: This should be a subtype but not everything is typed covariant. children, null, - onError, + handleFlightError, options ? options.identifierPrefix : undefined, undefined, 'Markup', @@ -153,7 +182,7 @@ export function renderToMarkup( ), createRootFormatContext(), Infinity, - onError, + handleError, undefined, undefined, undefined, diff --git a/packages/react-html/src/__tests__/ReactHTMLClient-test.js b/packages/react-html/src/__tests__/ReactHTMLClient-test.js index 02cef97c2d82f..5fb0a55acc8d8 100644 --- a/packages/react-html/src/__tests__/ReactHTMLClient-test.js +++ b/packages/react-html/src/__tests__/ReactHTMLClient-test.js @@ -12,6 +12,15 @@ let React; let ReactHTML; +function normalizeCodeLocInfo(str) { + return ( + str && + String(str).replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function (m, name) { + return '\n in ' + name + ' (at **)'; + }) + ); +} + if (!__EXPERIMENTAL__) { it('should not be built in stable', () => { try { @@ -170,5 +179,58 @@ if (!__EXPERIMENTAL__) { const html = await ReactHTML.renderToMarkup(); expect(html).toBe('
01
'); }); + + it('can get the component owner stacks for onError in dev', async () => { + const thrownError = new Error('hi'); + const caughtErrors = []; + + function Foo() { + return ; + } + function Bar() { + return ( +
+ +
+ ); + } + function Baz({unused}) { + throw thrownError; + } + + await expect(async () => { + await ReactHTML.renderToMarkup( +
+ +
, + { + onError(error, errorInfo) { + caughtErrors.push({ + error: error, + parentStack: errorInfo.componentStack, + ownerStack: React.captureOwnerStack + ? React.captureOwnerStack() + : null, + }); + }, + }, + ); + }).rejects.toThrow(thrownError); + + expect(caughtErrors.length).toBe(1); + expect(caughtErrors[0].error).toBe(thrownError); + expect(normalizeCodeLocInfo(caughtErrors[0].parentStack)).toBe( + '\n in Baz (at **)' + + '\n in div (at **)' + + '\n in Bar (at **)' + + '\n in Foo (at **)' + + '\n in div (at **)', + ); + expect(normalizeCodeLocInfo(caughtErrors[0].ownerStack)).toBe( + __DEV__ && gate(flags => flags.enableOwnerStacks) + ? '\n in Bar (at **)' + '\n in Foo (at **)' + : null, + ); + }); }); } diff --git a/packages/react-html/src/__tests__/ReactHTMLServer-test.js b/packages/react-html/src/__tests__/ReactHTMLServer-test.js index 236c2b0f4021c..0d7b7c8da2dfc 100644 --- a/packages/react-html/src/__tests__/ReactHTMLServer-test.js +++ b/packages/react-html/src/__tests__/ReactHTMLServer-test.js @@ -15,6 +15,15 @@ global.TextEncoder = require('util').TextEncoder; let React; let ReactHTML; +function normalizeCodeLocInfo(str) { + return ( + str && + String(str).replace(/\n +(?:at|in) ([\S]+)[^\n]*/g, function (m, name) { + return '\n in ' + name + ' (at **)'; + }) + ); +} + if (!__EXPERIMENTAL__) { it('should not be built in stable', () => { try { @@ -200,5 +209,54 @@ if (!__EXPERIMENTAL__) { ); expect(html).toBe('
00
'); }); + + it('can get the component owner stacks for onError in dev', async () => { + const thrownError = new Error('hi'); + const caughtErrors = []; + + function Foo() { + return React.createElement(Bar); + } + function Bar() { + return React.createElement('div', null, React.createElement(Baz)); + } + function Baz({unused}) { + throw thrownError; + } + + await expect(async () => { + await ReactHTML.renderToMarkup( + React.createElement('div', null, React.createElement(Foo)), + { + onError(error, errorInfo) { + caughtErrors.push({ + error: error, + parentStack: errorInfo.componentStack, + ownerStack: React.captureOwnerStack + ? React.captureOwnerStack() + : null, + }); + }, + }, + ); + }).rejects.toThrow(thrownError); + + expect(caughtErrors.length).toBe(1); + expect(caughtErrors[0].error).toBe(thrownError); + expect(normalizeCodeLocInfo(caughtErrors[0].parentStack)).toBe( + // TODO: Because Fizz doesn't yet implement debugInfo for parent stacks + // it doesn't have the Server Components in the parent stacks. + '\n in Lazy (at **)' + + '\n in div (at **)' + + '\n in div (at **)', + ); + expect(normalizeCodeLocInfo(caughtErrors[0].ownerStack)).toBe( + __DEV__ && gate(flags => flags.enableOwnerStacks) + ? // TODO: Because Fizz doesn't yet implement debugInfo for parent stacks + // it doesn't have the Server Components in the parent stacks. + '\n in Lazy (at **)' + : null, + ); + }); }); }