From f2b6bf7c86f01ca194914e0e4305d8f430024a8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 12 Mar 2021 18:21:02 -0500 Subject: [PATCH] [Fizz] Destroy the stream with an error if the root throws (#20992) * Destroy the stream with an error if the root throws But not if the error happens inside a suspense boundary. * Try rewriting the test to see if it works in other Node envs --- .../ReactDOMFizzServerBrowser-test.js | 65 ++++++++++++++ .../__tests__/ReactDOMFizzServerNode-test.js | 86 +++++++++++++++++-- .../src/ReactNoopFlightServer.js | 1 + .../src/ReactNoopServer.js | 1 + packages/react-server/src/ReactFizzServer.js | 6 +- .../src/ReactServerStreamConfigBrowser.js | 15 ++++ .../src/ReactServerStreamConfigNode.js | 5 ++ .../forks/ReactServerStreamConfig.custom.js | 1 + 8 files changed, 174 insertions(+), 6 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js index b100f1b5a68d6..69a6e705ee6f7 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js @@ -15,6 +15,7 @@ global.TextEncoder = require('util').TextEncoder; let React; let ReactDOMFizzServer; +let Suspense; describe('ReactDOMFizzServer', () => { beforeEach(() => { @@ -23,8 +24,18 @@ describe('ReactDOMFizzServer', () => { if (__EXPERIMENTAL__) { ReactDOMFizzServer = require('react-dom/unstable-fizz.browser'); } + Suspense = React.Suspense; }); + const theError = new Error('This is an error'); + function Throw() { + throw theError; + } + const theInfinitePromise = new Promise(() => {}); + function InfiniteSuspend() { + throw theInfinitePromise; + } + async function readResult(stream) { const reader = stream.getReader(); let result = ''; @@ -45,4 +56,58 @@ describe('ReactDOMFizzServer', () => { const result = await readResult(stream); expect(result).toBe('
hello world
'); }); + + // @gate experimental + it('should error the stream when an error is thrown at the root', async () => { + const stream = ReactDOMFizzServer.renderToReadableStream( +
+ +
, + ); + + let caughtError = null; + let result = ''; + try { + result = await readResult(stream); + } catch (x) { + caughtError = x; + } + expect(caughtError).toBe(theError); + expect(result).toBe(''); + }); + + // @gate experimental + it('should error the stream when an error is thrown inside a fallback', async () => { + const stream = ReactDOMFizzServer.renderToReadableStream( +
+ }> + + +
, + ); + + let caughtError = null; + let result = ''; + try { + result = await readResult(stream); + } catch (x) { + caughtError = x; + } + expect(caughtError).toBe(theError); + expect(result).toBe(''); + }); + + // @gate experimental + it('should not error the stream when an error is thrown inside suspense boundary', async () => { + const stream = ReactDOMFizzServer.renderToReadableStream( +
+ Loading
}> + + + , + ); + + const result = await readResult(stream); + expect(result).toContain('Loading'); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js index ff09f64540294..1b1c5c4a314ac 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js @@ -13,6 +13,7 @@ let Stream; let React; let ReactDOMFizzServer; +let Suspense; describe('ReactDOMFizzServer', () => { beforeEach(() => { @@ -22,21 +23,96 @@ describe('ReactDOMFizzServer', () => { ReactDOMFizzServer = require('react-dom/unstable-fizz'); } Stream = require('stream'); + Suspense = React.Suspense; }); function getTestWritable() { const writable = new Stream.PassThrough(); writable.setEncoding('utf8'); - writable.result = ''; - writable.on('data', chunk => (writable.result += chunk)); - return writable; + const output = {result: '', error: undefined}; + writable.on('data', chunk => { + output.result += chunk; + }); + writable.on('error', error => { + output.error = error; + }); + const completed = new Promise(resolve => { + writable.on('finish', () => { + resolve(); + }); + writable.on('error', () => { + resolve(); + }); + }); + return {writable, completed, output}; + } + + const theError = new Error('This is an error'); + function Throw() { + throw theError; + } + const theInfinitePromise = new Promise(() => {}); + function InfiniteSuspend() { + throw theInfinitePromise; } // @gate experimental it('should call pipeToNodeWritable', () => { - const writable = getTestWritable(); + const {writable, output} = getTestWritable(); ReactDOMFizzServer.pipeToNodeWritable(
hello world
, writable); jest.runAllTimers(); - expect(writable.result).toBe('
hello world
'); + expect(output.result).toBe('
hello world
'); + }); + + // @gate experimental + it('should error the stream when an error is thrown at the root', async () => { + const {writable, output, completed} = getTestWritable(); + ReactDOMFizzServer.pipeToNodeWritable( +
+ +
, + writable, + ); + + await completed; + + expect(output.error).toBe(theError); + expect(output.result).toBe(''); + }); + + // @gate experimental + it('should error the stream when an error is thrown inside a fallback', async () => { + const {writable, output, completed} = getTestWritable(); + ReactDOMFizzServer.pipeToNodeWritable( +
+ }> + + +
, + writable, + ); + + await completed; + + expect(output.error).toBe(theError); + expect(output.result).toBe(''); + }); + + // @gate experimental + it('should not error the stream when an error is thrown inside suspense boundary', async () => { + const {writable, output, completed} = getTestWritable(); + ReactDOMFizzServer.pipeToNodeWritable( +
+ Loading
}> + + + , + writable, + ); + + await completed; + + expect(output.error).toBe(undefined); + expect(output.result).toContain('Loading'); }); }); diff --git a/packages/react-noop-renderer/src/ReactNoopFlightServer.js b/packages/react-noop-renderer/src/ReactNoopFlightServer.js index bbace47e2eb4b..411e6094164c3 100644 --- a/packages/react-noop-renderer/src/ReactNoopFlightServer.js +++ b/packages/react-noop-renderer/src/ReactNoopFlightServer.js @@ -32,6 +32,7 @@ const ReactNoopFlightServer = ReactFlightServer({ }, completeWriting(destination: Destination): void {}, close(destination: Destination): void {}, + closeWithError(destination: Destination, error: mixed): void {}, flushBuffered(destination: Destination): void {}, convertStringToBuffer(content: string): Uint8Array { return Buffer.from(content, 'utf8'); diff --git a/packages/react-noop-renderer/src/ReactNoopServer.js b/packages/react-noop-renderer/src/ReactNoopServer.js index 3be95ab051692..d4b60739c619f 100644 --- a/packages/react-noop-renderer/src/ReactNoopServer.js +++ b/packages/react-noop-renderer/src/ReactNoopServer.js @@ -74,6 +74,7 @@ const ReactNoopServer = ReactFizzServer({ }, completeWriting(destination: Destination): void {}, close(destination: Destination): void {}, + closeWithError(destination: Destination, error: mixed): void {}, flushBuffered(destination: Destination): void {}, createResponseState(): null { diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 26bc6b2f4d6af..4f5879b86bff5 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -22,6 +22,7 @@ import { completeWriting, flushBuffered, close, + closeWithError, } from './ReactServerStreamConfig'; import { writePlaceholder, @@ -205,7 +206,7 @@ function fatalError(request: Request, error: mixed): void { // a suspense boundary or if the root suspense boundary's fallback errors. // It's also called if React itself or its host configs errors. request.status = CLOSED; - // TODO: Destroy the stream with an error. We weren't able to complete the root. + closeWithError(request.destination, error); } function renderNode( @@ -237,6 +238,7 @@ function renderNode( // Something suspended, we'll need to create a new segment and resolve it later. const insertionIndex = segment.chunks.length; const newSegment = createPendingSegment(request, insertionIndex, null); + segment.children.push(newSegment); const suspendedWork = createSuspendedWork( request, node, @@ -273,6 +275,7 @@ function renderNode( insertionIndex, newBoundary, ); + segment.children.push(boundarySegment); // We create suspended work for the fallback because we don't want to actually work // on it yet in case we finish the main content, so we queue for later. const suspendedFallbackWork = createSuspendedWork( @@ -326,6 +329,7 @@ function errorWork( fatalError(request, error); } else if (!boundary.forceClientRender) { boundary.forceClientRender = true; + // Regardless of what happens next, this boundary won't be displayed, // so we can flush it, if the parent already flushed. if (boundary.parentFlushed) { diff --git a/packages/react-server/src/ReactServerStreamConfigBrowser.js b/packages/react-server/src/ReactServerStreamConfigBrowser.js index aa17656ceebe8..efcef78509cb0 100644 --- a/packages/react-server/src/ReactServerStreamConfigBrowser.js +++ b/packages/react-server/src/ReactServerStreamConfigBrowser.js @@ -39,3 +39,18 @@ const textEncoder = new TextEncoder(); export function convertStringToBuffer(content: string): Uint8Array { return textEncoder.encode(content); } + +export function closeWithError(destination: Destination, error: mixed): void { + if (typeof destination.error === 'function') { + // $FlowFixMe: This is an Error object or the destination accepts other types. + destination.error(error); + } else { + // Earlier implementations doesn't support this method. In that environment you're + // supposed to throw from a promise returned but we don't return a promise in our + // approach. We could fork this implementation but this is environment is an edge + // case to begin with. It's even less common to run this in an older environment. + // Even then, this is not where errors are supposed to happen and they get reported + // to a global callback in addition to this anyway. So it's fine just to close this. + destination.close(); + } +} diff --git a/packages/react-server/src/ReactServerStreamConfigNode.js b/packages/react-server/src/ReactServerStreamConfigNode.js index 87c37ffab677c..00e3364fc93a5 100644 --- a/packages/react-server/src/ReactServerStreamConfigNode.js +++ b/packages/react-server/src/ReactServerStreamConfigNode.js @@ -64,3 +64,8 @@ export function close(destination: Destination) { export function convertStringToBuffer(content: string): Uint8Array { return Buffer.from(content, 'utf8'); } + +export function closeWithError(destination: Destination, error: mixed): void { + // $FlowFixMe: This is an Error object or the destination accepts other types. + destination.destroy(error); +} diff --git a/packages/react-server/src/forks/ReactServerStreamConfig.custom.js b/packages/react-server/src/forks/ReactServerStreamConfig.custom.js index c3dc6d83d64b0..2766595967375 100644 --- a/packages/react-server/src/forks/ReactServerStreamConfig.custom.js +++ b/packages/react-server/src/forks/ReactServerStreamConfig.custom.js @@ -32,4 +32,5 @@ export const writeChunk = $$$hostConfig.writeChunk; export const completeWriting = $$$hostConfig.completeWriting; export const flushBuffered = $$$hostConfig.flushBuffered; export const close = $$$hostConfig.close; +export const closeWithError = $$$hostConfig.closeWithError; export const convertStringToBuffer = $$$hostConfig.convertStringToBuffer;