Skip to content

Commit f2b6bf7

Browse files
sebmarkbagesebmarkbage
authored
[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
1 parent 10cc400 commit f2b6bf7

8 files changed

+174
-6
lines changed

‎packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js

+65
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ global.TextEncoder = require('util').TextEncoder;
1515

1616
let React;
1717
let ReactDOMFizzServer;
18+
let Suspense;
1819

1920
describe('ReactDOMFizzServer', () => {
2021
beforeEach(() => {
@@ -23,8 +24,18 @@ describe('ReactDOMFizzServer', () => {
2324
if (__EXPERIMENTAL__) {
2425
ReactDOMFizzServer = require('react-dom/unstable-fizz.browser');
2526
}
27+
Suspense = React.Suspense;
2628
});
2729

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+
2839
async function readResult(stream) {
2940
const reader = stream.getReader();
3041
let result = '';
@@ -45,4 +56,58 @@ describe('ReactDOMFizzServer', () => {
4556
const result = await readResult(stream);
4657
expect(result).toBe('<div>hello world</div>');
4758
});
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+
});
48113
});

‎packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js

+81-5
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
let Stream;
1414
let React;
1515
let ReactDOMFizzServer;
16+
let Suspense;
1617

1718
describe('ReactDOMFizzServer', () => {
1819
beforeEach(() => {
@@ -22,21 +23,96 @@ describe('ReactDOMFizzServer', () => {
2223
ReactDOMFizzServer = require('react-dom/unstable-fizz');
2324
}
2425
Stream = require('stream');
26+
Suspense = React.Suspense;
2527
});
2628

2729
function getTestWritable() {
2830
const writable = new Stream.PassThrough();
2931
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;
3357
}
3458

3559
// @gate experimental
3660
it('should call pipeToNodeWritable', () => {
37-
const writable = getTestWritable();
61+
const {writable, output} = getTestWritable();
3862
ReactDOMFizzServer.pipeToNodeWritable(<div>hello world</div>, writable);
3963
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');
41117
});
42118
});

‎packages/react-noop-renderer/src/ReactNoopFlightServer.js

+1
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ const ReactNoopFlightServer = ReactFlightServer({
3232
},
3333
completeWriting(destination: Destination): void {},
3434
close(destination: Destination): void {},
35+
closeWithError(destination: Destination, error: mixed): void {},
3536
flushBuffered(destination: Destination): void {},
3637
convertStringToBuffer(content: string): Uint8Array {
3738
return Buffer.from(content, 'utf8');

‎packages/react-noop-renderer/src/ReactNoopServer.js

+1
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ const ReactNoopServer = ReactFizzServer({
7474
},
7575
completeWriting(destination: Destination): void {},
7676
close(destination: Destination): void {},
77+
closeWithError(destination: Destination, error: mixed): void {},
7778
flushBuffered(destination: Destination): void {},
7879

7980
createResponseState(): null {

‎packages/react-server/src/ReactFizzServer.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
completeWriting,
2323
flushBuffered,
2424
close,
25+
closeWithError,
2526
} from './ReactServerStreamConfig';
2627
import {
2728
writePlaceholder,
@@ -205,7 +206,7 @@ function fatalError(request: Request, error: mixed): void {
205206
// a suspense boundary or if the root suspense boundary's fallback errors.
206207
// It's also called if React itself or its host configs errors.
207208
request.status = CLOSED;
208-
// TODO: Destroy the stream with an error. We weren't able to complete the root.
209+
closeWithError(request.destination, error);
209210
}
210211

211212
function renderNode(
@@ -237,6 +238,7 @@ function renderNode(
237238
// Something suspended, we'll need to create a new segment and resolve it later.
238239
const insertionIndex = segment.chunks.length;
239240
const newSegment = createPendingSegment(request, insertionIndex, null);
241+
segment.children.push(newSegment);
240242
const suspendedWork = createSuspendedWork(
241243
request,
242244
node,
@@ -273,6 +275,7 @@ function renderNode(
273275
insertionIndex,
274276
newBoundary,
275277
);
278+
segment.children.push(boundarySegment);
276279
// We create suspended work for the fallback because we don't want to actually work
277280
// on it yet in case we finish the main content, so we queue for later.
278281
const suspendedFallbackWork = createSuspendedWork(
@@ -326,6 +329,7 @@ function errorWork(
326329
fatalError(request, error);
327330
} else if (!boundary.forceClientRender) {
328331
boundary.forceClientRender = true;
332+
329333
// Regardless of what happens next, this boundary won't be displayed,
330334
// so we can flush it, if the parent already flushed.
331335
if (boundary.parentFlushed) {

‎packages/react-server/src/ReactServerStreamConfigBrowser.js

+15
Original file line numberDiff line numberDiff line change
@@ -39,3 +39,18 @@ const textEncoder = new TextEncoder();
3939
export function convertStringToBuffer(content: string): Uint8Array {
4040
return textEncoder.encode(content);
4141
}
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
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,8 @@ export function close(destination: Destination) {
6464
export function convertStringToBuffer(content: string): Uint8Array {
6565
return Buffer.from(content, 'utf8');
6666
}
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
Original file line numberDiff line numberDiff line change
@@ -32,4 +32,5 @@ export const writeChunk = $$$hostConfig.writeChunk;
3232
export const completeWriting = $$$hostConfig.completeWriting;
3333
export const flushBuffered = $$$hostConfig.flushBuffered;
3434
export const close = $$$hostConfig.close;
35+
export const closeWithError = $$$hostConfig.closeWithError;
3536
export const convertStringToBuffer = $$$hostConfig.convertStringToBuffer;

0 commit comments

Comments
 (0)