Skip to content

[Fizz] Send errors down to client #24202

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

Closed
Closed
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
113 changes: 90 additions & 23 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,34 @@ describe('ReactDOMFizzServer', () => {
});
});

function expectErrors(errorsArr, toBeDevArr, toBeProdArr) {
if (__DEV__) {
expect(errorsArr.map(error => normalizeCodeLocInfo(error))).toEqual(
toBeDevArr.map(error => {
if (typeof error === 'string' || error instanceof String) {
return error;
}
let str = JSON.stringify(error).replace(/\\n/g, '\n');
// this gets stripped away by normalizeCodeLocInfo...
// Kind of hacky but lets strip it away here too just so they match...
// easier than fixing the regex to account for this edge case
if (str.endsWith('at **)"}')) {
str = str.replace(/at \*\*\)\"}$/, 'at **)');
}
return str;
}),
);
} else {
expect(errorsArr).toEqual(toBeProdArr);
}
}

function componentStack(components) {
return components
.map(component => `\n in ${component} (at **)`)
.join('');
}

async function act(callback) {
await callback();
// Await one turn around the event loop.
Expand Down Expand Up @@ -421,12 +449,13 @@ describe('ReactDOMFizzServer', () => {
}

let bootstrapped = false;
const errors = [];
window.__INIT__ = function() {
bootstrapped = true;
// Attempt to hydrate the content.
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
errors.push(error.message);
},
});
};
Expand All @@ -438,6 +467,7 @@ describe('ReactDOMFizzServer', () => {
bootstrapScriptContent: '__INIT__();',
onError(x) {
loggedErrors.push(x);
return 'Hash';
},
},
);
Expand All @@ -464,10 +494,17 @@ describe('ReactDOMFizzServer', () => {
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

// Now we can client render it instead.
expect(Scheduler).toFlushAndYield([
'The server could not finish this Suspense boundary, likely due to ' +
'an error during server rendering. Switched to client rendering.',
]);
expect(Scheduler).toFlushAndYield([]);
expectErrors(
errors,
[
{
error: theError.message,
componentStack: componentStack(['Lazy', 'Suspense', 'div', 'App']),
},
],
['Hash'],
);

// The client rendered HTML is now in place.
expect(getVisibleChildren(container)).toEqual(<div>Hello</div>);
Expand Down Expand Up @@ -534,17 +571,19 @@ describe('ReactDOMFizzServer', () => {
{
onError(x) {
loggedErrors.push(x);
return 'hash';
},
},
);
pipe(writable);
});
expect(loggedErrors).toEqual([]);

const errors = [];
// Attempt to hydrate the content.
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
errors.push(error.message);
},
});
Scheduler.unstable_flushAll();
Expand All @@ -565,10 +604,18 @@ describe('ReactDOMFizzServer', () => {
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

// Now we can client render it instead.
expect(Scheduler).toFlushAndYield([
'The server could not finish this Suspense boundary, likely due to ' +
'an error during server rendering. Switched to client rendering.',
]);
expect(Scheduler).toFlushAndYield([]);

expectErrors(
errors,
[
{
error: theError.message,
componentStack: componentStack(['div', 'App']),
},
],
['hash'],
);

// The client rendered HTML is now in place.
expect(getVisibleChildren(container)).toEqual(<div>Hello</div>);
Expand Down Expand Up @@ -852,10 +899,11 @@ describe('ReactDOMFizzServer', () => {

// We're still showing a fallback.

const errors = [];
// Attempt to hydrate the content.
ReactDOMClient.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
errors.push(error.message);
},
});
Scheduler.unstable_flushAll();
Expand All @@ -869,10 +917,12 @@ describe('ReactDOMFizzServer', () => {
});

// We still can't render it on the client.
expect(Scheduler).toFlushAndYield([
'The server could not finish this Suspense boundary, likely due to an ' +
'error during server rendering. Switched to client rendering.',
]);
expect(Scheduler).toFlushAndYield([]);
expectErrors(
errors,
['This Suspense boundary was aborted by the server'],
['This Suspense boundary was aborted by the server'],
);
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

// We now resolve it on the client.
Expand Down Expand Up @@ -1540,6 +1590,7 @@ describe('ReactDOMFizzServer', () => {
{
onError(x) {
loggedErrors.push(x);
return 'error hash';
},
},
);
Expand All @@ -1548,10 +1599,11 @@ describe('ReactDOMFizzServer', () => {

// We're still showing a fallback.

const errors = [];
// Attempt to hydrate the content.
ReactDOMClient.hydrateRoot(container, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
errors.push(error.message);
},
});
Scheduler.unstable_flushAll();
Expand Down Expand Up @@ -1582,10 +1634,24 @@ describe('ReactDOMFizzServer', () => {
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

// That will let us client render it instead.
expect(Scheduler).toFlushAndYield([
'The server could not finish this Suspense boundary, likely due to ' +
'an error during server rendering. Switched to client rendering.',
]);
expect(Scheduler).toFlushAndYield([]);
expectErrors(
errors,
[
{
error: theError.message,
componentStack: componentStack([
'AsyncText',
'h1',
'Suspense',
'div',
'Suspense',
'App',
]),
},
],
['error hash'],
);

// The client rendered HTML is now in place.
expect(getVisibleChildren(container)).toEqual(
Expand Down Expand Up @@ -2205,18 +2271,19 @@ describe('ReactDOMFizzServer', () => {

// Hydrate the tree. Child will throw during render.
isClient = true;
const errors = [];
ReactDOMClient.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(
'Log recoverable error: ' + error.message,
);
errors.push(error.message);
},
});

// Because we failed to recover from the error, onRecoverableError
// shouldn't be called.
expect(Scheduler).toFlushAndYield([]);
expect(getVisibleChildren(container)).toEqual('Oops!');

expectErrors(errors, [], []);
},
);

Expand Down
5 changes: 5 additions & 0 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,11 @@ export function isSuspenseInstancePending(instance: SuspenseInstance) {
export function isSuspenseInstanceFallback(instance: SuspenseInstance) {
return instance.data === SUSPENSE_FALLBACK_START_DATA;
}
export function getSuspenseInstanceFallbackError(
instance: SuspenseInstance,
): string {
return (instance: any).data2;
}

export function registerSuspenseInstanceRetry(
instance: SuspenseInstance,
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/server/ReactDOMFizzServerBrowser.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type Options = {|
bootstrapModules?: Array<string>,
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => void,
onError?: (error: mixed) => ?string,
|};

// TODO: Move to sub-classing ReadableStream.
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/server/ReactDOMFizzServerNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type Options = {|
onShellReady?: () => void,
onShellError?: () => void,
onAllReady?: () => void,
onError?: (error: mixed) => void,
onError?: (error: mixed) => string,
|};

type Controls = {|
Expand Down
17 changes: 12 additions & 5 deletions packages/react-dom/src/server/ReactDOMServerFormatConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -1681,7 +1681,7 @@ export function writeEndSegment(
// const SUSPENSE_PENDING_START_DATA = '$?';
// const SUSPENSE_FALLBACK_START_DATA = '$!';
//
// function clientRenderBoundary(suspenseBoundaryID) {
// function clientRenderBoundary(suspenseBoundaryID, errorMsg) {
// // Find the fallback's first element.
// const suspenseIdNode = document.getElementById(suspenseBoundaryID);
// if (!suspenseIdNode) {
Expand All @@ -1693,6 +1693,7 @@ export function writeEndSegment(
// const suspenseNode = suspenseIdNode.previousSibling;
// // Tag it to be client rendered.
// suspenseNode.data = SUSPENSE_FALLBACK_START_DATA;
// suspenseNode.data2 = errorMsg;
// // Tell React to retry it if the parent already hydrated.
// if (suspenseNode._reactRetry) {
// suspenseNode._reactRetry();
Expand Down Expand Up @@ -1780,7 +1781,7 @@ const completeSegmentFunction =
const completeBoundaryFunction =
'function $RC(a,b){a=document.getElementById(a);b=document.getElementById(b);b.parentNode.removeChild(b);if(a){a=a.previousSibling;var f=a.parentNode,c=a.nextSibling,e=0;do{if(c&&8===c.nodeType){var d=c.data;if("/$"===d)if(0===e)break;else e--;else"$"!==d&&"$?"!==d&&"$!"!==d||e++}d=c.nextSibling;f.removeChild(c);c=d}while(c);for(;b.firstChild;)f.insertBefore(b.firstChild,c);a.data="$";a._reactRetry&&a._reactRetry()}}';
const clientRenderFunction =
'function $RX(a){if(a=document.getElementById(a))a=a.previousSibling,a.data="$!",a._reactRetry&&a._reactRetry()}';
'function $RX(a,b){if(a=document.getElementById(a))a=a.previousSibling,a.data="$!",a.data2=b,a._reactRetry&&a._reactRetry()}';

const completeSegmentScript1Full = stringToPrecomputedChunk(
completeSegmentFunction + ';$RS("',
Expand Down Expand Up @@ -1850,15 +1851,17 @@ export function writeCompletedBoundaryInstruction(
}

const clientRenderScript1Full = stringToPrecomputedChunk(
clientRenderFunction + ';$RX("',
clientRenderFunction + ";$RX('",
);
const clientRenderScript1Partial = stringToPrecomputedChunk('$RX("');
const clientRenderScript2 = stringToPrecomputedChunk('")</script>');
const clientRenderScript1Partial = stringToPrecomputedChunk("$RX('");
const clientRenderScript2 = stringToPrecomputedChunk("')</script>");
const clientRenderErrorScript1 = stringToPrecomputedChunk("','");

export function writeClientRenderBoundaryInstruction(
destination: Destination,
responseState: ResponseState,
boundaryID: SuspenseBoundaryID,
error: ?string,
): boolean {
writeChunk(destination, responseState.startInlineScript);
if (!responseState.sentClientRenderFunction) {
Expand All @@ -1877,5 +1880,9 @@ export function writeClientRenderBoundaryInstruction(
}

writeChunk(destination, boundaryID);
if (error) {
writeChunk(destination, clientRenderErrorScript1);
writeChunk(destination, stringToChunk(error));
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the error message sneaks in a user space string, that can be used to insert tags like </script><script>alert('p0wned'). The approach chosen in #23063 won't work here by itself unless we also encode the whole script tag rather than just this string.

Copy link
Collaborator

@sebmarkbage sebmarkbage Apr 20, 2022

Choose a reason for hiding this comment

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

Maybe if we assume that the rest of the script doesn't contain </script> or comments, it's ok to only escape script tags but it needs vetting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, fully escaping it should be fine since this should be a string anyways

}
return writeChunkAndReturn(destination, clientRenderScript2);
}
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,8 @@ export function writeClientRenderBoundaryInstruction(
destination: Destination,
responseState: ResponseState,
boundaryID: SuspenseBoundaryID,
// TODO: encode error for native
error: ?string,
): boolean {
writeChunk(destination, SUSPENSE_UPDATE_TO_CLIENT_RENDER);
return writeChunkAndReturn(destination, formatID(boundaryID));
Expand Down
2 changes: 1 addition & 1 deletion packages/react-noop-renderer/src/ReactNoopServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ type Options = {
progressiveChunkSize?: number,
onShellReady?: () => void,
onAllReady?: () => void,
onError?: (error: mixed) => void,
onError?: (error: mixed) => ?string,
};

function render(children: React$Element<any>, options?: Options): Destination {
Expand Down
15 changes: 10 additions & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ import {
shouldSetTextContent,
isSuspenseInstancePending,
isSuspenseInstanceFallback,
getSuspenseInstanceFallbackError,
registerSuspenseInstanceRetry,
supportsHydration,
isPrimaryRenderer,
Expand Down Expand Up @@ -2689,18 +2690,22 @@ function updateDehydratedSuspenseComponent(
// This boundary is in a permanent fallback state. In this case, we'll never
// get an update and we'll never be able to hydrate the final content. Let's just try the
// client side render instead.
const errorMsg = getSuspenseInstanceFallbackError(suspenseInstance);
return retrySuspenseComponentWithoutHydrating(
current,
workInProgress,
renderLanes,
// TODO: The server should serialize the error message so we can log it
// here on the client. Or, in production, a hash/id that corresponds to
// the error.
new Error(
'The server could not finish this Suspense boundary, likely ' +
'due to an error during server rendering. Switched to ' +
'client rendering.',
),
errorMsg
? // eslint-disable-next-line react-internal/prod-error-codes
new Error(errorMsg)
: new Error(
'The server could not finish this Suspense boundary, likely ' +
'due to an error during server rendering. Switched to ' +
'client rendering.',
),
);
}

Expand Down
Loading