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

[Fizz] handle errors in onHeaders #27712

Merged
merged 1 commit into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
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