Skip to content

Commit

Permalink
[Fizz] Send errors down to client (#24551)
Browse files Browse the repository at this point in the history
* use return from onError

* export getSuspenseInstanceFallbackError

* stringToChunk

* return string from onError in downstream type signatures

* 1 more type

* support encoding errors in html stream and escape user input

This commit adds another way to get errors to the suspense instance by encoding them as dataset properties of a template element at the head of the boundary. Previously if there was an error before the boundary flushed there was no way to stream the error to the client because there would never be a client render instruction.

Additionally the error is sent in 3 parts

1) error hash - this is always sent (dev or prod) if one is provided
2) error message - Dev only
3) error component stack - Dev only, this now captures the stack at the point of error

Another item addressed in this commit is the escaping of potentially unsafe data. all error components are escaped as test for browers when written into the html and as javascript strings when written into a client render instruction.

* nits

Co-authored-by: Marco Salazar <[email protected]>
  • Loading branch information
gnoff and salazarm authored May 30, 2022
1 parent a276638 commit aec5759
Show file tree
Hide file tree
Showing 17 changed files with 768 additions and 77 deletions.
457 changes: 418 additions & 39 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -193,20 +193,30 @@ describe('ReactDOMFizzServer', () => {

// @gate experimental
it('should be able to complete by aborting even if the promise never resolves', async () => {
const errors = [];
const controller = new AbortController();
const stream = await ReactDOMFizzServer.renderToReadableStream(
<div>
<Suspense fallback={<div>Loading</div>}>
<InfiniteSuspend />
</Suspense>
</div>,
{signal: controller.signal},
{
signal: controller.signal,
onError(x) {
errors.push(x.message);
},
},
);

controller.abort();

const result = await readResult(stream);
expect(result).toContain('Loading');

expect(errors).toEqual([
'This Suspense boundary was aborted by the server',
]);
});

// @gate experimental
Expand All @@ -223,12 +233,18 @@ describe('ReactDOMFizzServer', () => {
rendered = true;
return 'Done';
}
const errors = [];
const stream = await ReactDOMFizzServer.renderToReadableStream(
<div>
<Suspense fallback={<div>Loading</div>}>
<Wait /> />
</Suspense>
</div>,
{
onError(x) {
errors.push(x.message);
},
},
);

stream.allReady.then(() => (isComplete = true));
Expand All @@ -239,6 +255,10 @@ describe('ReactDOMFizzServer', () => {
const reader = stream.getReader();
reader.cancel();

expect(errors).toEqual([
'This Suspense boundary was aborted by the server',
]);

hasLoaded = true;
resolve();

Expand Down
30 changes: 28 additions & 2 deletions packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ describe('ReactDOMFizzServer', () => {

{
onError(x) {
reportedErrors.push(x);
reportedErrors.push(x.message);
},
onShellError(x) {
reportedShellErrors.push(x);
Expand All @@ -224,7 +224,10 @@ describe('ReactDOMFizzServer', () => {

expect(output.error).toBe(theError);
expect(output.result).toBe('');
expect(reportedErrors).toEqual([theError]);
expect(reportedErrors).toEqual([
theError.message,
'This Suspense boundary was aborted by the server',
]);
expect(reportedShellErrors).toEqual([theError]);
});

Expand Down Expand Up @@ -289,6 +292,7 @@ describe('ReactDOMFizzServer', () => {
// @gate experimental
it('should be able to complete by aborting even if the promise never resolves', async () => {
let isCompleteCalls = 0;
const errors = [];
const {writable, output, completed} = getTestWritable();
const {pipe, abort} = ReactDOMFizzServer.renderToPipeableStream(
<div>
Expand All @@ -298,6 +302,9 @@ describe('ReactDOMFizzServer', () => {
</div>,

{
onError(x) {
errors.push(x.message);
},
onAllReady() {
isCompleteCalls++;
},
Expand All @@ -314,6 +321,9 @@ describe('ReactDOMFizzServer', () => {

await completed;

expect(errors).toEqual([
'This Suspense boundary was aborted by the server',
]);
expect(output.error).toBe(undefined);
expect(output.result).toContain('Loading');
expect(isCompleteCalls).toBe(1);
Expand All @@ -322,6 +332,7 @@ describe('ReactDOMFizzServer', () => {
// @gate experimental
it('should be able to complete by abort when the fallback is also suspended', async () => {
let isCompleteCalls = 0;
const errors = [];
const {writable, output, completed} = getTestWritable();
const {pipe, abort} = ReactDOMFizzServer.renderToPipeableStream(
<div>
Expand All @@ -333,6 +344,9 @@ describe('ReactDOMFizzServer', () => {
</div>,

{
onError(x) {
errors.push(x.message);
},
onAllReady() {
isCompleteCalls++;
},
Expand All @@ -349,6 +363,11 @@ describe('ReactDOMFizzServer', () => {

await completed;

expect(errors).toEqual([
// There are two boundaries that abort
'This Suspense boundary was aborted by the server',
'This Suspense boundary was aborted by the server',
]);
expect(output.error).toBe(undefined);
expect(output.result).toContain('Loading');
expect(isCompleteCalls).toBe(1);
Expand Down Expand Up @@ -552,6 +571,7 @@ describe('ReactDOMFizzServer', () => {
rendered = true;
return 'Done';
}
const errors = [];
const {writable, completed} = getTestWritable();
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
<div>
Expand All @@ -560,6 +580,9 @@ describe('ReactDOMFizzServer', () => {
</Suspense>
</div>,
{
onError(x) {
errors.push(x.message);
},
onAllReady() {
isComplete = true;
},
Expand All @@ -579,6 +602,9 @@ describe('ReactDOMFizzServer', () => {

await completed;

expect(errors).toEqual([
'This Suspense boundary was aborted by the server',
]);
expect(rendered).toBe(false);
expect(isComplete).toBe(true);
});
Expand Down
24 changes: 24 additions & 0 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,30 @@ export function isSuspenseInstancePending(instance: SuspenseInstance) {
export function isSuspenseInstanceFallback(instance: SuspenseInstance) {
return instance.data === SUSPENSE_FALLBACK_START_DATA;
}
export function getSuspenseInstanceFallbackErrorDetails(
instance: SuspenseInstance,
) {
const nextSibling = instance.nextSibling;
let errorMessage /*, errorComponentStack, errorHash*/;
if (
nextSibling &&
nextSibling.nodeType === ELEMENT_NODE &&
nextSibling.nodeName.toLowerCase() === 'template'
) {
const msg = ((nextSibling: any): HTMLTemplateElement).dataset.msg;
if (msg !== null) errorMessage = msg;

// @TODO read and return hash and componentStack once we know how we are goign to
// expose this extra errorInfo to onRecoverableError

// const hash = ((nextSibling: any): HTMLTemplateElement).dataset.hash;
// if (hash !== null) errorHash = hash;

// const stack = ((nextSibling: any): HTMLTemplateElement).dataset.stack;
// if (stack !== null) errorComponentStack = stack;
}
return {errorMessage /*, errorComponentStack, errorHash*/};
}

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?: (error: mixed) => void,
onAllReady?: () => void,
onError?: (error: mixed) => void,
onError?: (error: mixed) => ?string,
|};

type PipeableStream = {|
Expand Down
110 changes: 106 additions & 4 deletions packages/react-dom/src/server/ReactDOMServerFormatConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -1526,6 +1526,19 @@ const startClientRenderedSuspenseBoundary = stringToPrecomputedChunk(
);
const endSuspenseBoundary = stringToPrecomputedChunk('<!--/$-->');

const clientRenderedSuspenseBoundaryError1 = stringToPrecomputedChunk(
'<template data-hash="',
);
const clientRenderedSuspenseBoundaryError1A = stringToPrecomputedChunk(
'" data-msg="',
);
const clientRenderedSuspenseBoundaryError1B = stringToPrecomputedChunk(
'" data-stack="',
);
const clientRenderedSuspenseBoundaryError2 = stringToPrecomputedChunk(
'"></template>',
);

export function pushStartCompletedSuspenseBoundary(
target: Array<Chunk | PrecomputedChunk>,
) {
Expand Down Expand Up @@ -1563,8 +1576,43 @@ export function writeStartPendingSuspenseBoundary(
export function writeStartClientRenderedSuspenseBoundary(
destination: Destination,
responseState: ResponseState,
errorHash: ?string,
errorMesssage: ?string,
errorComponentStack: ?string,
): boolean {
return writeChunkAndReturn(destination, startClientRenderedSuspenseBoundary);
let result;
result = writeChunkAndReturn(
destination,
startClientRenderedSuspenseBoundary,
);
if (errorHash) {
writeChunk(destination, clientRenderedSuspenseBoundaryError1);
writeChunk(destination, stringToChunk(escapeTextForBrowser(errorHash)));
// In prod errorMessage will usually be nullish but there is one case where
// it is used (currently when the server aborts the task) so we leave it ungated.
if (errorMesssage) {
writeChunk(destination, clientRenderedSuspenseBoundaryError1A);
writeChunk(
destination,
stringToChunk(escapeTextForBrowser(errorMesssage)),
);
}
if (__DEV__) {
// Component stacks are currently only captured in dev
if (errorComponentStack) {
writeChunk(destination, clientRenderedSuspenseBoundaryError1B);
writeChunk(
destination,
stringToChunk(escapeTextForBrowser(errorComponentStack)),
);
}
}
result = writeChunkAndReturn(
destination,
clientRenderedSuspenseBoundaryError2,
);
}
return result;
}
export function writeEndCompletedSuspenseBoundary(
destination: Destination,
Expand Down Expand Up @@ -1724,7 +1772,7 @@ export function writeEndSegment(
// const SUSPENSE_PENDING_START_DATA = '$?';
// const SUSPENSE_FALLBACK_START_DATA = '$!';
//
// function clientRenderBoundary(suspenseBoundaryID) {
// function clientRenderBoundary(suspenseBoundaryID, errorHash, errorMsg, errorComponentStack) {
// // Find the fallback's first element.
// const suspenseIdNode = document.getElementById(suspenseBoundaryID);
// if (!suspenseIdNode) {
Expand All @@ -1736,6 +1784,11 @@ export function writeEndSegment(
// const suspenseNode = suspenseIdNode.previousSibling;
// // Tag it to be client rendered.
// suspenseNode.data = SUSPENSE_FALLBACK_START_DATA;
// // assign error metadata to first sibling
// let dataset = suspenseIdNode.dataset;
// if (errorHash) dataset.hash = errorHash;
// if (errorMsg) dataset.msg = errorMsg;
// if (errorComponentStack) dataset.stack = errorComponentStack;
// // Tell React to retry it if the parent already hydrated.
// if (suspenseNode._reactRetry) {
// suspenseNode._reactRetry();
Expand Down Expand Up @@ -1823,7 +1876,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(b,c,d,e){var a=document.getElementById(b);a&&(b=a.previousSibling,b.data="$!",a=a.dataset,c&&(a.hash=c),d&&(a.msg=d),e&&(a.stack=e),b._reactRetry&&b._reactRetry())}';

const completeSegmentScript1Full = stringToPrecomputedChunk(
completeSegmentFunction + ';$RS("',
Expand Down Expand Up @@ -1896,12 +1949,17 @@ const clientRenderScript1Full = stringToPrecomputedChunk(
clientRenderFunction + ';$RX("',
);
const clientRenderScript1Partial = stringToPrecomputedChunk('$RX("');
const clientRenderScript2 = stringToPrecomputedChunk('")</script>');
const clientRenderScript1A = stringToPrecomputedChunk('"');
const clientRenderScript2 = stringToPrecomputedChunk(')</script>');
const clientRenderErrorScriptArgInterstitial = stringToPrecomputedChunk(',');

export function writeClientRenderBoundaryInstruction(
destination: Destination,
responseState: ResponseState,
boundaryID: SuspenseBoundaryID,
errorHash: ?string,
errorMessage?: string,
errorComponentStack?: string,
): boolean {
writeChunk(destination, responseState.startInlineScript);
if (!responseState.sentClientRenderFunction) {
Expand All @@ -1920,5 +1978,49 @@ export function writeClientRenderBoundaryInstruction(
}

writeChunk(destination, boundaryID);
writeChunk(destination, clientRenderScript1A);
if (errorHash || errorMessage || errorComponentStack) {
writeChunk(destination, clientRenderErrorScriptArgInterstitial);
writeChunk(
destination,
stringToChunk(escapeJSStringsForInstructionScripts(errorHash || '')),
);
}
if (errorMessage || errorComponentStack) {
writeChunk(destination, clientRenderErrorScriptArgInterstitial);
writeChunk(
destination,
stringToChunk(escapeJSStringsForInstructionScripts(errorMessage || '')),
);
}
if (errorComponentStack) {
writeChunk(destination, clientRenderErrorScriptArgInterstitial);
writeChunk(
destination,
stringToChunk(escapeJSStringsForInstructionScripts(errorComponentStack)),
);
}
return writeChunkAndReturn(destination, clientRenderScript2);
}

const regexForJSStringsInScripts = /[<\u2028\u2029]/g;
function escapeJSStringsForInstructionScripts(input: string): string {
const escaped = JSON.stringify(input);
return escaped.replace(regexForJSStringsInScripts, match => {
switch (match) {
// santizing breaking out of strings and script tags
case '<':
return '\\u003c';
case '\u2028':
return '\\u2028';
case '\u2029':
return '\\u2029';
default: {
// eslint-disable-next-line react-internal/prod-error-codes
throw new Error(
'escapeJSStringsForInstructionScripts encountered a match it does not know how to replace. this means the match regex and the replacement characters are no longer in sync. This is a bug in React',
);
}
}
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,10 @@ export function writeStartCompletedSuspenseBoundary(
export function writeStartClientRenderedSuspenseBoundary(
destination: Destination,
responseState: ResponseState,
// flushing these error arguments are not currently supported in this legacy streaming format.
errorHash: ?string,
errorMessage?: string,
errorComponentStack?: string,
): boolean {
if (responseState.generateStaticMarkup) {
// A client rendered boundary is done and doesn't need a representation in the HTML
Expand Down
Loading

0 comments on commit aec5759

Please sign in to comment.