From 5690932765b65998ea9f054f0740002257b7675c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Tue, 8 Feb 2022 22:38:14 -0500 Subject: [PATCH] Add onErrorShell Callback (#23247) This indicates that an error has happened before the shell completed and there's no point in emitting the result of this stream. This is not quite the same as other fatal errors that can happen even after streaming as started. It's also not quite the same as onError before onCompleteShell because onError can be called for an error inside a Suspense boundary before the shell completes. Implement shell error handling in Node SSR fixtures Instead of hanging indefinitely. Update Browser Fixture Expose onErrorShell to the Node build This API is not Promisified so it's just a separate callback instead. Promisify the Browser Fizz API It's now a Promise of a readable stream. The Promise resolves when the shell completes. If the shell errors, the Promise is rejected. --- fixtures/fizz-ssr-browser/index.html | 36 +++++--- fixtures/ssr/server/render.js | 5 ++ fixtures/ssr2/server/render.js | 5 ++ .../ReactDOMFizzServerBrowser-test.js | 76 ++++++++--------- .../__tests__/ReactDOMFizzServerNode-test.js | 15 ++++ .../src/server/ReactDOMFizzServerBrowser.js | 85 ++++++++++--------- .../src/server/ReactDOMFizzServerNode.js | 2 + packages/react-server/src/ReactFizzServer.js | 9 ++ 8 files changed, 137 insertions(+), 96 deletions(-) diff --git a/fixtures/fizz-ssr-browser/index.html b/fixtures/fizz-ssr-browser/index.html index 82ed7bf6145da..3d2b42231005b 100644 --- a/fixtures/fizz-ssr-browser/index.html +++ b/fixtures/fizz-ssr-browser/index.html @@ -20,22 +20,29 @@

Fizz Example

diff --git a/fixtures/ssr/server/render.js b/fixtures/ssr/server/render.js index 9857a8a83dcfa..ae30e622174cb 100644 --- a/fixtures/ssr/server/render.js +++ b/fixtures/ssr/server/render.js @@ -28,6 +28,11 @@ export default function render(url, res) { res.setHeader('Content-type', 'text/html'); pipe(res); }, + onErrorShell(x) { + // Something errored before we could complete the shell so we emit an alternative shell. + res.statusCode = 500; + res.send('

Error

'); + }, onError(x) { didError = true; console.error(x); diff --git a/fixtures/ssr2/server/render.js b/fixtures/ssr2/server/render.js index fa91f763f2902..ccd12af0d3638 100644 --- a/fixtures/ssr2/server/render.js +++ b/fixtures/ssr2/server/render.js @@ -49,6 +49,11 @@ module.exports = function render(url, res) { res.setHeader('Content-type', 'text/html'); pipe(res); }, + onErrorShell(x) { + // Something errored before we could complete the shell so we emit an alternative shell. + res.statusCode = 500; + res.send('

Error

'); + }, onError(x) { didError = true; console.error(x); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js index 53a17c8b6ec23..87d04bdd07655 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js @@ -51,7 +51,7 @@ describe('ReactDOMFizzServer', () => { // @gate experimental it('should call renderToReadableStream', async () => { - const stream = ReactDOMFizzServer.renderToReadableStream( + const stream = await ReactDOMFizzServer.renderToReadableStream(
hello world
, ); const result = await readResult(stream); @@ -60,7 +60,7 @@ describe('ReactDOMFizzServer', () => { // @gate experimental it('should emit DOCTYPE at the root of the document', async () => { - const stream = ReactDOMFizzServer.renderToReadableStream( + const stream = await ReactDOMFizzServer.renderToReadableStream( hello world , @@ -73,7 +73,7 @@ describe('ReactDOMFizzServer', () => { // @gate experimental it('should emit bootstrap script src at the end', async () => { - const stream = ReactDOMFizzServer.renderToReadableStream( + const stream = await ReactDOMFizzServer.renderToReadableStream(
hello world
, { bootstrapScriptContent: 'INIT();', @@ -99,7 +99,7 @@ describe('ReactDOMFizzServer', () => { return 'Done'; } let isComplete = false; - const stream = ReactDOMFizzServer.renderToReadableStream( + const stream = await ReactDOMFizzServer.renderToReadableStream(
@@ -128,63 +128,55 @@ describe('ReactDOMFizzServer', () => { }); // @gate experimental - it('should error the stream when an error is thrown at the root', async () => { + it('should reject the promise when an error is thrown at the root', async () => { const reportedErrors = []; - const stream = ReactDOMFizzServer.renderToReadableStream( -
- -
, - { - onError(x) { - reportedErrors.push(x); - }, - }, - ); - let caughtError = null; - let result = ''; try { - result = await readResult(stream); - } catch (x) { - caughtError = x; + await ReactDOMFizzServer.renderToReadableStream( +
+ +
, + { + onError(x) { + reportedErrors.push(x); + }, + }, + ); + } catch (error) { + caughtError = error; } expect(caughtError).toBe(theError); - expect(result).toBe(''); expect(reportedErrors).toEqual([theError]); }); // @gate experimental - it('should error the stream when an error is thrown inside a fallback', async () => { + it('should reject the promise when an error is thrown inside a fallback', async () => { const reportedErrors = []; - const stream = ReactDOMFizzServer.renderToReadableStream( -
- }> - - -
, - { - onError(x) { - reportedErrors.push(x); - }, - }, - ); - let caughtError = null; - let result = ''; try { - result = await readResult(stream); - } catch (x) { - caughtError = x; + await ReactDOMFizzServer.renderToReadableStream( +
+ }> + + +
, + { + onError(x) { + reportedErrors.push(x); + }, + }, + ); + } catch (error) { + caughtError = error; } expect(caughtError).toBe(theError); - expect(result).toBe(''); expect(reportedErrors).toEqual([theError]); }); // @gate experimental it('should not error the stream when an error is thrown inside suspense boundary', async () => { const reportedErrors = []; - const stream = ReactDOMFizzServer.renderToReadableStream( + const stream = await ReactDOMFizzServer.renderToReadableStream(
Loading
}> @@ -205,7 +197,7 @@ describe('ReactDOMFizzServer', () => { // @gate experimental it('should be able to complete by aborting even if the promise never resolves', async () => { const controller = new AbortController(); - const stream = ReactDOMFizzServer.renderToReadableStream( + const stream = await ReactDOMFizzServer.renderToReadableStream(
Loading
}> diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js index a453e0347d8d5..a35a250282ec4 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js @@ -168,6 +168,7 @@ describe('ReactDOMFizzServer', () => { // @gate experimental it('should error the stream when an error is thrown at the root', async () => { const reportedErrors = []; + const reportedShellErrors = []; const {writable, output, completed} = getTestWritable(); const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
@@ -178,6 +179,9 @@ describe('ReactDOMFizzServer', () => { onError(x) { reportedErrors.push(x); }, + onErrorShell(x) { + reportedShellErrors.push(x); + }, }, ); @@ -190,11 +194,13 @@ describe('ReactDOMFizzServer', () => { expect(output.result).toBe(''); // This type of error is reported to the error callback too. expect(reportedErrors).toEqual([theError]); + expect(reportedShellErrors).toEqual([theError]); }); // @gate experimental it('should error the stream when an error is thrown inside a fallback', async () => { const reportedErrors = []; + const reportedShellErrors = []; const {writable, output, completed} = getTestWritable(); const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
@@ -207,6 +213,9 @@ describe('ReactDOMFizzServer', () => { onError(x) { reportedErrors.push(x); }, + onErrorShell(x) { + reportedShellErrors.push(x); + }, }, ); pipe(writable); @@ -216,11 +225,13 @@ describe('ReactDOMFizzServer', () => { expect(output.error).toBe(theError); expect(output.result).toBe(''); expect(reportedErrors).toEqual([theError]); + expect(reportedShellErrors).toEqual([theError]); }); // @gate experimental it('should not error the stream when an error is thrown inside suspense boundary', async () => { const reportedErrors = []; + const reportedShellErrors = []; const {writable, output, completed} = getTestWritable(); const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
@@ -233,6 +244,9 @@ describe('ReactDOMFizzServer', () => { onError(x) { reportedErrors.push(x); }, + onErrorShell(x) { + reportedShellErrors.push(x); + }, }, ); pipe(writable); @@ -243,6 +257,7 @@ describe('ReactDOMFizzServer', () => { expect(output.result).toContain('Loading'); // While no error is reported to the stream, the error is reported to the callback. expect(reportedErrors).toEqual([theError]); + expect(reportedShellErrors).toEqual([]); }); // @gate experimental diff --git a/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js b/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js index 907f0823cffe9..15fc69437ca15 100644 --- a/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js +++ b/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js @@ -32,7 +32,6 @@ type Options = {| bootstrapModules?: Array, progressiveChunkSize?: number, signal?: AbortSignal, - onCompleteShell?: () => void, onCompleteAll?: () => void, onError?: (error: mixed) => void, |}; @@ -40,46 +39,52 @@ type Options = {| function renderToReadableStream( children: ReactNodeList, options?: Options, -): ReadableStream { - const request = createRequest( - children, - createResponseState( - options ? options.identifierPrefix : undefined, - options ? options.nonce : undefined, - options ? options.bootstrapScriptContent : undefined, - options ? options.bootstrapScripts : undefined, - options ? options.bootstrapModules : undefined, - ), - createRootFormatContext(options ? options.namespaceURI : undefined), - options ? options.progressiveChunkSize : undefined, - options ? options.onError : undefined, - options ? options.onCompleteAll : undefined, - options ? options.onCompleteShell : undefined, - ); - if (options && options.signal) { - const signal = options.signal; - const listener = () => { - abort(request); - signal.removeEventListener('abort', listener); - }; - signal.addEventListener('abort', listener); - } - const stream = new ReadableStream({ - start(controller) { - startWork(request); - }, - pull(controller) { - // Pull is called immediately even if the stream is not passed to anything. - // That's buffering too early. We want to start buffering once the stream - // is actually used by something so we can give it the best result possible - // at that point. - if (stream.locked) { - startFlowing(request, controller); - } - }, - cancel(reason) {}, +): Promise { + return new Promise((resolve, reject) => { + function onCompleteShell() { + const stream = new ReadableStream({ + pull(controller) { + // Pull is called immediately even if the stream is not passed to anything. + // That's buffering too early. We want to start buffering once the stream + // is actually used by something so we can give it the best result possible + // at that point. + if (stream.locked) { + startFlowing(request, controller); + } + }, + cancel(reason) {}, + }); + resolve(stream); + } + function onErrorShell(error: mixed) { + reject(error); + } + const request = createRequest( + children, + createResponseState( + options ? options.identifierPrefix : undefined, + options ? options.nonce : undefined, + options ? options.bootstrapScriptContent : undefined, + options ? options.bootstrapScripts : undefined, + options ? options.bootstrapModules : undefined, + ), + createRootFormatContext(options ? options.namespaceURI : undefined), + options ? options.progressiveChunkSize : undefined, + options ? options.onError : undefined, + options ? options.onCompleteAll : undefined, + onCompleteShell, + onErrorShell, + ); + if (options && options.signal) { + const signal = options.signal; + const listener = () => { + abort(request); + signal.removeEventListener('abort', listener); + }; + signal.addEventListener('abort', listener); + } + startWork(request); }); - return stream; } export {renderToReadableStream, ReactVersion as version}; diff --git a/packages/react-dom/src/server/ReactDOMFizzServerNode.js b/packages/react-dom/src/server/ReactDOMFizzServerNode.js index 33a7083bb95eb..15e2636187065 100644 --- a/packages/react-dom/src/server/ReactDOMFizzServerNode.js +++ b/packages/react-dom/src/server/ReactDOMFizzServerNode.js @@ -37,6 +37,7 @@ type Options = {| bootstrapModules?: Array, progressiveChunkSize?: number, onCompleteShell?: () => void, + onErrorShell?: () => void, onCompleteAll?: () => void, onError?: (error: mixed) => void, |}; @@ -63,6 +64,7 @@ function createRequestImpl(children: ReactNodeList, options: void | Options) { options ? options.onError : undefined, options ? options.onCompleteAll : undefined, options ? options.onCompleteShell : undefined, + options ? options.onErrorShell : undefined, ); } diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 2cf1e5c2fb31d..e46aee9a45cd2 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -199,6 +199,9 @@ export opaque type Request = { // Typically you don't need this callback because it's best practice to always have a // root fallback ready so there's no need to wait. onCompleteShell: () => void, + // onErrorShell is called when the shell didn't complete. That means you probably want to + // emit a different response to the stream instead. + onErrorShell: (error: mixed) => void, }; // This is a default heuristic for how to split up the HTML content into progressive @@ -232,6 +235,7 @@ export function createRequest( onError: void | ((error: mixed) => void), onCompleteAll: void | (() => void), onCompleteShell: void | (() => void), + onErrorShell: void | ((error: mixed) => void), ): Request { const pingedTasks = []; const abortSet: Set = new Set(); @@ -256,6 +260,7 @@ export function createRequest( onError: onError === undefined ? defaultErrorHandler : onError, onCompleteAll: onCompleteAll === undefined ? noop : onCompleteAll, onCompleteShell: onCompleteShell === undefined ? noop : onCompleteShell, + onErrorShell: onErrorShell === undefined ? noop : onErrorShell, }; // This segment represents the root fallback. const rootSegment = createPendingSegment(request, 0, null, rootFormatContext); @@ -412,6 +417,8 @@ function fatalError(request: Request, error: mixed): void { // This is called outside error handling code such as if the root errors outside // a suspense boundary or if the root suspense boundary's fallback errors. // It's also called if React itself or its host configs errors. + const onErrorShell = request.onErrorShell; + onErrorShell(error); if (request.destination !== null) { request.status = CLOSED; closeWithError(request.destination, error); @@ -1433,6 +1440,8 @@ function finishedTask( } request.pendingRootTasks--; if (request.pendingRootTasks === 0) { + // We have completed the shell so the shell can't error anymore. + request.onErrorShell = noop; const onCompleteShell = request.onCompleteShell; onCompleteShell(); }