Skip to content

Commit

Permalink
[Fizz] Add Component Stacks to onError and onPostpone when in dev…
Browse files Browse the repository at this point in the history
… mode or during prerenders in prod mode (#27761)

Historically React would produce component stacks for dev builds only.
There is a cost to tracking component stacks and given the prod builds
try to optimize runtime performance these stacks were left out. More
recently React added production component stacks to Fiber in because it
can be immensely helpful in tracking down hard to debug production
issues. Fizz was not updated to have a similar behavior.

With the advent of prerendering however stacks for production in Fizz
are more relevant because prerendering is not really a dev-time task. If
you want the ability to reason about errors or postpones that happen
during a prerender having component stacks to interrogate is helpful and
these component stacks need to be available in production otherwise you
are really never going to see them. (it is possible that you could do
dev-mode prerenders but we don't expect this to be a common dev mode
workflow)

To better support the prerender use case and to make error logging in
Fizz more useful the following changes have been made

1. `onPostpone` now accepts a second `postponeInfo` argument which will
contain a componentStack. Postpones always originate from a component
render so the stack should be consistently available. The type however
will indicate the stack is optional so we can remove them in the future
if we decide the overhead is the wrong tradeoff in certain cases
2. `onError` now accepts a second `errorInfo` argument which may contain
a componentStack. If an error originated from a component a stack will
be included in the following cases.

This change entails tracking the component hierarchy in prod builds now.
While this isn't cost free it is implemented in a relatively lean
manner. Deferring the most expensive work (reifying the stack) until we
are actually in an error pathway.

In the course of implementing this change a number of simplifications
were made to the code which should make the stack tracking more
resilient. We no longer use a module global to curry the stack up to
some handler. This was delicate because you needed to always reset it
properly. We now curry the stack on the task itself.

Another change made was to track the component stack on SuspenseBoundary
instances so that we can provide the stack when aborting suspense
boundaries to help you determine which ones were affected by an abort.
  • Loading branch information
gnoff committed Dec 16, 2023
1 parent 493610f commit 63310df
Show file tree
Hide file tree
Showing 9 changed files with 348 additions and 294 deletions.
53 changes: 45 additions & 8 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@ describe('ReactDOMFizzServer', () => {

const theError = new Error('Test');
const loggedErrors = [];
function onError(x) {
function onError(x, errorInfo) {
loggedErrors.push(x);
return 'Hash of (' + x.message + ')';
}
Expand Down Expand Up @@ -837,7 +837,7 @@ describe('ReactDOMFizzServer', () => {

const theError = new Error('Test');
const loggedErrors = [];
function onError(x) {
function onError(x, errorInfo) {
loggedErrors.push(x);
return 'hash of (' + x.message + ')';
}
Expand Down Expand Up @@ -898,7 +898,7 @@ describe('ReactDOMFizzServer', () => {
[
theError.message,
expectedDigest,
componentStack(['Suspense', 'div', 'App']),
componentStack(['Lazy', 'Suspense', 'div', 'App']),
],
],
[
Expand Down Expand Up @@ -936,7 +936,9 @@ describe('ReactDOMFizzServer', () => {
return (
<div>
<Suspense fallback={<span>loading...</span>}>
<Erroring isClient={isClient} />
<Indirection level={2}>
<Erroring isClient={isClient} />
</Indirection>
</Suspense>
</div>
);
Expand Down Expand Up @@ -979,7 +981,15 @@ describe('ReactDOMFizzServer', () => {
[
theError.message,
expectedDigest,
componentStack(['Erroring', 'Suspense', 'div', 'App']),
componentStack([
'Erroring',
'Indirection',
'Indirection',
'Indirection',
'Suspense',
'div',
'App',
]),
],
],
[
Expand Down Expand Up @@ -1330,6 +1340,11 @@ describe('ReactDOMFizzServer', () => {
<AsyncText text="Hello" />
</h1>
</Suspense>
<main>
<Suspense fallback="loading...">
<AsyncText text="World" />
</Suspense>
</main>
</div>
);
}
Expand Down Expand Up @@ -1359,7 +1374,11 @@ describe('ReactDOMFizzServer', () => {
await waitForAll([]);

// We're still loading because we're waiting for the server to stream more content.
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
expect(getVisibleChildren(container)).toEqual(
<div>
Loading...<main>loading...</main>
</div>,
);

// We abort the server response.
await act(() => {
Expand All @@ -1374,26 +1393,44 @@ describe('ReactDOMFizzServer', () => {
[
'The server did not finish this Suspense boundary: The render was aborted by the server without a reason.',
expectedDigest,
// We get the stack of the task when it was aborted which is why we see `h1`
componentStack(['h1', 'Suspense', 'div', 'App']),
],
[
'The server did not finish this Suspense boundary: The render was aborted by the server without a reason.',
expectedDigest,
componentStack(['Suspense', 'main', 'div', 'App']),
],
],
[
[
'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
expectedDigest,
],
[
'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
expectedDigest,
],
],
);
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
expect(getVisibleChildren(container)).toEqual(
<div>
Loading...<main>loading...</main>
</div>,
);

// We now resolve it on the client.
await clientAct(() => resolveText('Hello'));
await clientAct(() => {
resolveText('Hello');
resolveText('World');
});
assertLog([]);

// The client rendered HTML is now in place.
expect(getVisibleChildren(container)).toEqual(
<div>
<h1>Hello</h1>
<main>World</main>
</div>,
);
});
Expand Down
10 changes: 7 additions & 3 deletions packages/react-dom/src/server/ReactDOMFizzServerBrowser.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
* @flow
*/

import type {PostponedState} from 'react-server/src/ReactFizzServer';
import type {
PostponedState,
ErrorInfo,
PostponeInfo,
} from 'react-server/src/ReactFizzServer';
import type {ReactNodeList, ReactFormState} from 'shared/ReactTypes';
import type {
BootstrapScriptDescriptor,
Expand Down Expand Up @@ -42,8 +46,8 @@ type Options = {
bootstrapModules?: Array<string | BootstrapScriptDescriptor>,
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => ?string,
onPostpone?: (reason: string) => void,
onError?: (error: mixed, errorInfo: ErrorInfo) => ?string,
onPostpone?: (reason: string, postponeInfo: PostponeInfo) => void,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
importMap?: ImportMap,
formState?: ReactFormState<any, any> | null,
Expand Down
5 changes: 3 additions & 2 deletions packages/react-dom/src/server/ReactDOMFizzServerBun.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
HeadersDescriptor,
} from 'react-dom-bindings/src/server/ReactFizzConfigDOM';
import type {ImportMap} from '../shared/ReactDOMTypes';
import type {ErrorInfo, PostponeInfo} from 'react-server/src/ReactFizzServer';

import ReactVersion from 'shared/ReactVersion';

Expand All @@ -39,8 +40,8 @@ type Options = {
bootstrapModules?: Array<string | BootstrapScriptDescriptor>,
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => ?string,
onPostpone?: (reason: string) => void,
onError?: (error: mixed, errorInfo: ErrorInfo) => ?string,
onPostpone?: (reason: string, postponeInfo: PostponeInfo) => void,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
importMap?: ImportMap,
formState?: ReactFormState<any, any> | null,
Expand Down
10 changes: 7 additions & 3 deletions packages/react-dom/src/server/ReactDOMFizzServerEdge.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
* @flow
*/

import type {PostponedState} from 'react-server/src/ReactFizzServer';
import type {
PostponedState,
ErrorInfo,
PostponeInfo,
} from 'react-server/src/ReactFizzServer';
import type {ReactNodeList, ReactFormState} from 'shared/ReactTypes';
import type {
BootstrapScriptDescriptor,
Expand Down Expand Up @@ -42,8 +46,8 @@ type Options = {
bootstrapModules?: Array<string | BootstrapScriptDescriptor>,
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => ?string,
onPostpone?: (reason: string) => void,
onError?: (error: mixed, errorInfo: ErrorInfo) => ?string,
onPostpone?: (reason: string, postponeInfo: PostponeInfo) => void,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
importMap?: ImportMap,
formState?: ReactFormState<any, any> | null,
Expand Down
15 changes: 10 additions & 5 deletions packages/react-dom/src/server/ReactDOMFizzServerNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@
* @flow
*/

import type {Request, PostponedState} from 'react-server/src/ReactFizzServer';
import type {
Request,
PostponedState,
ErrorInfo,
PostponeInfo,
} from 'react-server/src/ReactFizzServer';
import type {ReactNodeList, ReactFormState} from 'shared/ReactTypes';
import type {Writable} from 'stream';
import type {
Expand Down Expand Up @@ -59,8 +64,8 @@ type Options = {
onShellReady?: () => void,
onShellError?: (error: mixed) => void,
onAllReady?: () => void,
onError?: (error: mixed) => ?string,
onPostpone?: (reason: string) => void,
onError?: (error: mixed, errorInfo: ErrorInfo) => ?string,
onPostpone?: (reason: string, postponeInfo: PostponeInfo) => void,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
importMap?: ImportMap,
formState?: ReactFormState<any, any> | null,
Expand All @@ -73,8 +78,8 @@ type ResumeOptions = {
onShellReady?: () => void,
onShellError?: (error: mixed) => void,
onAllReady?: () => void,
onError?: (error: mixed) => ?string,
onPostpone?: (reason: string) => void,
onError?: (error: mixed, errorInfo: ErrorInfo) => ?string,
onPostpone?: (reason: string, postponeInfo: PostponeInfo) => void,
};

type PipeableStream = {
Expand Down
10 changes: 7 additions & 3 deletions packages/react-dom/src/server/ReactDOMFizzStaticBrowser.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ import type {
BootstrapScriptDescriptor,
HeadersDescriptor,
} from 'react-dom-bindings/src/server/ReactFizzConfigDOM';
import type {PostponedState} from 'react-server/src/ReactFizzServer';
import type {
PostponedState,
ErrorInfo,
PostponeInfo,
} from 'react-server/src/ReactFizzServer';
import type {ImportMap} from '../shared/ReactDOMTypes';

import ReactVersion from 'shared/ReactVersion';
Expand Down Expand Up @@ -40,8 +44,8 @@ type Options = {
bootstrapModules?: Array<string | BootstrapScriptDescriptor>,
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => ?string,
onPostpone?: (reason: string) => void,
onError?: (error: mixed, errorInfo: ErrorInfo) => ?string,
onPostpone?: (reason: string, postponeInfo: PostponeInfo) => void,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
importMap?: ImportMap,
onHeaders?: (headers: Headers) => void,
Expand Down
10 changes: 7 additions & 3 deletions packages/react-dom/src/server/ReactDOMFizzStaticEdge.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ import type {
BootstrapScriptDescriptor,
HeadersDescriptor,
} from 'react-dom-bindings/src/server/ReactFizzConfigDOM';
import type {PostponedState} from 'react-server/src/ReactFizzServer';
import type {
PostponedState,
ErrorInfo,
PostponeInfo,
} from 'react-server/src/ReactFizzServer';
import type {ImportMap} from '../shared/ReactDOMTypes';

import ReactVersion from 'shared/ReactVersion';
Expand Down Expand Up @@ -40,8 +44,8 @@ type Options = {
bootstrapModules?: Array<string | BootstrapScriptDescriptor>,
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => ?string,
onPostpone?: (reason: string) => void,
onError?: (error: mixed, errorInfo: ErrorInfo) => ?string,
onPostpone?: (reason: string, postponeInfo: PostponeInfo) => void,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
importMap?: ImportMap,
onHeaders?: (headers: Headers) => void,
Expand Down
10 changes: 7 additions & 3 deletions packages/react-dom/src/server/ReactDOMFizzStaticNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ import type {
BootstrapScriptDescriptor,
HeadersDescriptor,
} from 'react-dom-bindings/src/server/ReactFizzConfigDOM';
import type {PostponedState} from 'react-server/src/ReactFizzServer';
import type {
PostponedState,
ErrorInfo,
PostponeInfo,
} from 'react-server/src/ReactFizzServer';
import type {ImportMap} from '../shared/ReactDOMTypes';

import {Writable, Readable} from 'stream';
Expand Down Expand Up @@ -41,8 +45,8 @@ type Options = {
bootstrapModules?: Array<string | BootstrapScriptDescriptor>,
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => ?string,
onPostpone?: (reason: string) => void,
onError?: (error: mixed, errorInfo: ErrorInfo) => ?string,
onPostpone?: (reason: string, postponeInfo: PostponeInfo) => void,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
importMap?: ImportMap,
onHeaders?: (headers: HeadersDescriptor) => void,
Expand Down
Loading

0 comments on commit 63310df

Please sign in to comment.