@samitouri / QOS-React-2 / commits / f2b6bf7c86

[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

Sebastian Markbåge committed Mar 12, 2021 at 18:21 UTC f2b6bf7c86f01ca194914e0e4305d8f430024a8c
8 files changed +174 -6
packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js
+65
@@ -15,6 +15,7 @@ global.TextEncoder = require('util').TextEncoder;
15
16 let React;
17 let ReactDOMFizzServer;
18 +let Suspense;
19
20 describe('ReactDOMFizzServer', () => {
21 beforeEach(() => {
@@ -23,8 +24,18 @@ describe('ReactDOMFizzServer', () => {
24 if (__EXPERIMENTAL__) {
25 ReactDOMFizzServer = require('react-dom/unstable-fizz.browser');
26 }
27 + Suspense = React.Suspense;
28 });
29
30 + const theError = new Error('This is an error');
31 + function Throw() {
32 + throw theError;
33 + }
34 + const theInfinitePromise = new Promise(() => {});
35 + function InfiniteSuspend() {
36 + throw theInfinitePromise;
37 + }
38 +
39 async function readResult(stream) {
40 const reader = stream.getReader();
41 let result = '';
@@ -45,4 +56,58 @@ describe('ReactDOMFizzServer', () => {
56 const result = await readResult(stream);
57 expect(result).toBe('<div>hello world</div>');
58 });
59 +
60 + // @gate experimental
61 + it('should error the stream when an error is thrown at the root', async () => {
62 + const stream = ReactDOMFizzServer.renderToReadableStream(
63 + <div>
64 + <Throw />
65 + </div>,
66 + );
67 +
68 + let caughtError = null;
69 + let result = '';
70 + try {
71 + result = await readResult(stream);
72 + } catch (x) {
73 + caughtError = x;
74 + }
75 + expect(caughtError).toBe(theError);
76 + expect(result).toBe('');
77 + });
78 +
79 + // @gate experimental
80 + it('should error the stream when an error is thrown inside a fallback', async () => {
81 + const stream = ReactDOMFizzServer.renderToReadableStream(
82 + <div>
83 + <Suspense fallback={<Throw />}>
84 + <InfiniteSuspend />
85 + </Suspense>
86 + </div>,
87 + );
88 +
89 + let caughtError = null;
90 + let result = '';
91 + try {
92 + result = await readResult(stream);
93 + } catch (x) {
94 + caughtError = x;
95 + }
96 + expect(caughtError).toBe(theError);
97 + expect(result).toBe('');
98 + });
99 +
100 + // @gate experimental
101 + it('should not error the stream when an error is thrown inside suspense boundary', async () => {
102 + const stream = ReactDOMFizzServer.renderToReadableStream(
103 + <div>
104 + <Suspense fallback={<div>Loading</div>}>
105 + <Throw />
106 + </Suspense>
107 + </div>,
108 + );
109 +
110 + const result = await readResult(stream);
111 + expect(result).toContain('Loading');
112 + });
113 });
packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js
+81 -5
@@ -13,6 +13,7 @@
13 let Stream;
14 let React;
15 let ReactDOMFizzServer;
16 +let Suspense;
17
18 describe('ReactDOMFizzServer', () => {
19 beforeEach(() => {
@@ -22,21 +23,96 @@ describe('ReactDOMFizzServer', () => {
23 ReactDOMFizzServer = require('react-dom/unstable-fizz');
24 }
25 Stream = require('stream');
26 + Suspense = React.Suspense;
27 });
28
29 function getTestWritable() {
30 const writable = new Stream.PassThrough();
31 writable.setEncoding('utf8');
30 - writable.result = '';
31 - writable.on('data', chunk => (writable.result += chunk));
32 - return writable;
32 + const output = {result: '', error: undefined};
33 + writable.on('data', chunk => {
34 + output.result += chunk;
35 + });
36 + writable.on('error', error => {
37 + output.error = error;
38 + });
39 + const completed = new Promise(resolve => {
40 + writable.on('finish', () => {
41 + resolve();
42 + });
43 + writable.on('error', () => {
44 + resolve();
45 + });
46 + });
47 + return {writable, completed, output};
48 + }
49 +
50 + const theError = new Error('This is an error');
51 + function Throw() {
52 + throw theError;
53 + }
54 + const theInfinitePromise = new Promise(() => {});
55 + function InfiniteSuspend() {
56 + throw theInfinitePromise;
57 }
58
59 // @gate experimental
60 it('should call pipeToNodeWritable', () => {
37 - const writable = getTestWritable();
61 + const {writable, output} = getTestWritable();
62 ReactDOMFizzServer.pipeToNodeWritable(<div>hello world</div>, writable);
63 jest.runAllTimers();
40 - expect(writable.result).toBe('<div>hello world</div>');
64 + expect(output.result).toBe('<div>hello world</div>');
65 + });
66 +
67 + // @gate experimental
68 + it('should error the stream when an error is thrown at the root', async () => {
69 + const {writable, output, completed} = getTestWritable();
70 + ReactDOMFizzServer.pipeToNodeWritable(
71 + <div>
72 + <Throw />
73 + </div>,
74 + writable,
75 + );
76 +
77 + await completed;
78 +
79 + expect(output.error).toBe(theError);
80 + expect(output.result).toBe('');
81 + });
82 +
83 + // @gate experimental
84 + it('should error the stream when an error is thrown inside a fallback', async () => {
85 + const {writable, output, completed} = getTestWritable();
86 + ReactDOMFizzServer.pipeToNodeWritable(
87 + <div>
88 + <Suspense fallback={<Throw />}>
89 + <InfiniteSuspend />
90 + </Suspense>
91 + </div>,
92 + writable,
93 + );
94 +
95 + await completed;
96 +
97 + expect(output.error).toBe(theError);
98 + expect(output.result).toBe('');
99 + });
100 +
101 + // @gate experimental
102 + it('should not error the stream when an error is thrown inside suspense boundary', async () => {
103 + const {writable, output, completed} = getTestWritable();
104 + ReactDOMFizzServer.pipeToNodeWritable(
105 + <div>
106 + <Suspense fallback={<div>Loading</div>}>
107 + <Throw />
108 + </Suspense>
109 + </div>,
110 + writable,
111 + );
112 +
113 + await completed;
114 +
115 + expect(output.error).toBe(undefined);
116 + expect(output.result).toContain('Loading');
117 });
118 });
packages/react-noop-renderer/src/ReactNoopFlightServer.js
+1
@@ -32,6 +32,7 @@ const ReactNoopFlightServer = ReactFlightServer({
32 },
33 completeWriting(destination: Destination): void {},
34 close(destination: Destination): void {},
35 + closeWithError(destination: Destination, error: mixed): void {},
36 flushBuffered(destination: Destination): void {},
37 convertStringToBuffer(content: string): Uint8Array {
38 return Buffer.from(content, 'utf8');
packages/react-noop-renderer/src/ReactNoopServer.js
+1
@@ -74,6 +74,7 @@ const ReactNoopServer = ReactFizzServer({
74 },
75 completeWriting(destination: Destination): void {},
76 close(destination: Destination): void {},
77 + closeWithError(destination: Destination, error: mixed): void {},
78 flushBuffered(destination: Destination): void {},
79
80 createResponseState(): null {
packages/react-server/src/ReactFizzServer.js
+5 -1
@@ -22,6 +22,7 @@ import {
22 completeWriting,
23 flushBuffered,
24 close,
25 + closeWithError,
26 } from './ReactServerStreamConfig';
27 import {
28 writePlaceholder,
@@ -205,7 +206,7 @@ function fatalError(request: Request, error: mixed): void {
206 // a suspense boundary or if the root suspense boundary's fallback errors.
207 // It's also called if React itself or its host configs errors.
208 request.status = CLOSED;
208 - // TODO: Destroy the stream with an error. We weren't able to complete the root.
209 + closeWithError(request.destination, error);
210 }
211
212 function renderNode(
@@ -237,6 +238,7 @@ function renderNode(
238 // Something suspended, we'll need to create a new segment and resolve it later.
239 const insertionIndex = segment.chunks.length;
240 const newSegment = createPendingSegment(request, insertionIndex, null);
241 + segment.children.push(newSegment);
242 const suspendedWork = createSuspendedWork(
243 request,
244 node,
@@ -273,6 +275,7 @@ function renderNode(
275 insertionIndex,
276 newBoundary,
277 );
278 + segment.children.push(boundarySegment);
279 // We create suspended work for the fallback because we don't want to actually work
280 // on it yet in case we finish the main content, so we queue for later.
281 const suspendedFallbackWork = createSuspendedWork(
@@ -326,6 +329,7 @@ function errorWork(
329 fatalError(request, error);
330 } else if (!boundary.forceClientRender) {
331 boundary.forceClientRender = true;
332 +
333 // Regardless of what happens next, this boundary won't be displayed,
334 // so we can flush it, if the parent already flushed.
335 if (boundary.parentFlushed) {
packages/react-server/src/ReactServerStreamConfigBrowser.js
+15
@@ -39,3 +39,18 @@ const textEncoder = new TextEncoder();
39 export function convertStringToBuffer(content: string): Uint8Array {
40 return textEncoder.encode(content);
41 }
42 +
43 +export function closeWithError(destination: Destination, error: mixed): void {
44 + if (typeof destination.error === 'function') {
45 + // $FlowFixMe: This is an Error object or the destination accepts other types.
46 + destination.error(error);
47 + } else {
48 + // Earlier implementations doesn't support this method. In that environment you're
49 + // supposed to throw from a promise returned but we don't return a promise in our
50 + // approach. We could fork this implementation but this is environment is an edge
51 + // case to begin with. It's even less common to run this in an older environment.
52 + // Even then, this is not where errors are supposed to happen and they get reported
53 + // to a global callback in addition to this anyway. So it's fine just to close this.
54 + destination.close();
55 + }
56 +}
packages/react-server/src/ReactServerStreamConfigNode.js
+5
@@ -64,3 +64,8 @@ export function close(destination: Destination) {
64 export function convertStringToBuffer(content: string): Uint8Array {
65 return Buffer.from(content, 'utf8');
66 }
67 +
68 +export function closeWithError(destination: Destination, error: mixed): void {
69 + // $FlowFixMe: This is an Error object or the destination accepts other types.
70 + destination.destroy(error);
71 +}
packages/react-server/src/forks/ReactServerStreamConfig.custom.js
+1
@@ -32,4 +32,5 @@ export const writeChunk = $$$hostConfig.writeChunk;
32 export const completeWriting = $$$hostConfig.completeWriting;
33 export const flushBuffered = $$$hostConfig.flushBuffered;
34 export const close = $$$hostConfig.close;
35 +export const closeWithError = $$$hostConfig.closeWithError;
36 export const convertStringToBuffer = $$$hostConfig.convertStringToBuffer;