Skip to content

Commit

Permalink
[Fizz] handle errors in onHeaders
Browse files Browse the repository at this point in the history
`onHeaders` can throw however for now we can assume that headers are optimistic values since the only things we produce for them are preload links. This is a pragmatic decision because React could concievably have headers in the future which were not optimistic and thus non-optional however it is hard to imagine what these headers might be in practice. If we need to change this behavior to be fatal in the future it would be a breaking change.

This commit adds error logging when `onHeaders` throws and ensures the request can continue to render successfully.
  • Loading branch information
gnoff committed Nov 15, 2023
1 parent aec521a commit 0ad7cc5
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 9 deletions.
5 changes: 4 additions & 1 deletion packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -6131,6 +6131,10 @@ export function emitEarlyPreloads(
if (onHeaders) {
const headers = renderState.headers;
if (headers) {
// Even if onHeaders throws we don't want to call this again so
// we drop the headers state from this point onwards.
renderState.headers = null;

let linkHeader = headers.preconnects;
if (headers.fontPreloads) {
if (linkHeader) {
Expand Down Expand Up @@ -6205,7 +6209,6 @@ export function emitEarlyPreloads(
// it React will not provide any headers
onHeaders({});
}
renderState.headers = null;
return;
}
}
Expand Down
24 changes: 24 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3841,6 +3841,30 @@ describe('ReactDOMFizzServer', () => {
});
});

it('logs an error if onHeaders throws but continues the render', async () => {
const errors = [];
function onError(error) {
errors.push(error.message);
}

function onHeaders(x) {
throw new Error('bad onHeaders');
}

let pipe;
await act(() => {
({pipe} = renderToPipeableStream(<div>hello</div>, {onHeaders, onError}));
});

expect(errors).toEqual(['bad onHeaders']);

await act(() => {
pipe(writable);
});

expect(getVisibleChildren(container)).toEqual(<div>hello</div>);
});

describe('error escaping', () => {
it('escapes error hash, message, and component stack values in directly flushed errors (html escaping)', async () => {
window.__outlet = {};
Expand Down
22 changes: 22 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1420,6 +1420,28 @@ describe('ReactDOMFizzStaticBrowser', () => {
);
});

// @gate experimental
it('logs an error if onHeaders throws but continues the prerender', async () => {
const errors = [];
function onError(error) {
errors.push(error.message);
}

function onHeaders(x) {
throw new Error('bad onHeaders');
}

const prerendered = await ReactDOMFizzStatic.prerender(<div>hello</div>, {
onHeaders,
onError,
});
expect(prerendered.postponed).toBe(null);
expect(errors).toEqual(['bad onHeaders']);

await readIntoContainer(prerendered.prelude);
expect(getVisibleChildren(container)).toEqual(<div>hello</div>);
});

// @gate enablePostpone
it('does not bootstrap again in a resume if it bootstraps', async () => {
let prerendering = true;
Expand Down
28 changes: 20 additions & 8 deletions packages/react-server/src/ReactFizzServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3220,6 +3220,22 @@ function abortTask(task: Task, request: Request, error: mixed): void {
}
}

function safelyEmitEarlyPreloads(
request: Request,
shellComplete: boolean,
): void {
try {
emitEarlyPreloads(
request.renderState,
request.resumableState,
shellComplete,
);
} catch (error) {
// We assume preloads are optimistic and thus non-fatal if errored.
logRecoverableError(request, error);
}
}

// I extracted this function out because we want to ensure we consistently emit preloads before
// transitioning to the next request stage and this transition can happen in multiple places in this
// implementation.
Expand All @@ -3232,11 +3248,7 @@ function completeShell(request: Request) {
// we should only be calling completeShell when the shell is complete so we
// just use a literal here
const shellComplete = true;
emitEarlyPreloads(
request.renderState,
request.resumableState,
shellComplete,
);
safelyEmitEarlyPreloads(request, shellComplete);
}
// We have completed the shell so the shell can't error anymore.
request.onShellError = noop;
Expand All @@ -3259,7 +3271,7 @@ function completeAll(request: Request) {
: // Prerender Request, we use the state of the root segment
request.completedRootSegment === null ||
request.completedRootSegment.status !== POSTPONED;
emitEarlyPreloads(request.renderState, request.resumableState, shellComplete);
safelyEmitEarlyPreloads(request, shellComplete);
const onAllReady = request.onAllReady;
onAllReady();
}
Expand Down Expand Up @@ -4124,7 +4136,7 @@ export function startWork(request: Request): void {

function enqueueEarlyPreloadsAfterInitialWork(request: Request) {
const shellComplete = request.pendingRootTasks === 0;
emitEarlyPreloads(request.renderState, request.resumableState, shellComplete);
safelyEmitEarlyPreloads(request, shellComplete);
}

function enqueueFlush(request: Request): void {
Expand Down Expand Up @@ -4168,7 +4180,7 @@ export function prepareForStartFlowingIfBeforeAllReady(request: Request) {
request.completedRootSegment === null
? request.pendingRootTasks === 0
: request.completedRootSegment.status !== POSTPONED;
emitEarlyPreloads(request.renderState, request.resumableState, shellComplete);
safelyEmitEarlyPreloads(request, shellComplete);
}

export function startFlowing(request: Request, destination: Destination): void {
Expand Down

0 comments on commit 0ad7cc5

Please sign in to comment.