Skip to content

Commit 2655c93

Browse files
Fizz Browser: fix precomputed chunk being cleared on Node 18 (#25645)
## Edit Went for another approach after talking with @gnoff. The approach is now: - add a dev-only error when a precomputed chunk is too big to be written - suggest to copy it before passing it to `writeChunk` This PR also includes porting the React Float tests to use the browser build of Fizz so that we can test it out on that environment (which is the one used by next). <!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn debug-test --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary Someone reported [a bug](vercel/next.js#42466) in Next.js that pointed to an issue with Node 18 in the streaming renderer when using importing a CSS module where it only returned a malformed bootstraping script only after loading the page once. After investigating a bit, here's what I found: - when using a CSS module in Next, we go into this code path, which writes the aforementioned bootstrapping script https://github.com/facebook/react/blob/5f7ef8c4cbe824ef126a947b7ae0e1c07b143357/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js#L2443-L2447 - the reason for the malformed script is that `completeBoundaryWithStylesScript1FullBoth` is emptied after the call to `writeChunk` - it gets emptied in `writeChunk` because we stream the chunk directly without copying it in this codepath https://github.com/facebook/react/blob/a438590144d2ad40865b58e0c0e69595fc1aa377/packages/react-server/src/ReactServerStreamConfigBrowser.js#L63 - the reason why it only happens from Node 18 is because the Webstreams APIs are available natively from that version and in their implementation, [`enqueue` transfers the array buffer ownership](https://github.com/nodejs/node/blob/9454ba6138d11e8a4d18b073de25781cad4bd2c8/lib/internal/webstreams/readablestream.js#L2641), thus making it unavailable/empty for subsequent calls. In older Node versions, we don't encounter the bug because we are using a polyfill in Next.js, [which does not implement properly the array buffer transfer behaviour](https://cs.github.com/MattiasBuelens/web-streams-polyfill/blob/d354a7457ca8a24030dbd0a135ee40baed7c774d/src/lib/abstract-ops/ecmascript.ts#L16). I think the proper fix for this is to clone the array buffer before enqueuing it. (we do this in the other code paths in the function later on, see ```((currentView: any): Uint8Array).set(bytesToWrite, writtenBytes);``` ## How did you test this change? Manually tested by applying the change in the compiled Next.js version. <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> Co-authored-by: Sebastian Markbage <[email protected]>
1 parent 799ee65 commit 2655c93

File tree

8 files changed

+81
-3
lines changed

8 files changed

+81
-3
lines changed

packages/react-dom-bindings/src/server/ReactDOMLegacyServerStreamConfig.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
5454
return content;
5555
}
5656

57+
export function clonePrecomputedChunk(
58+
chunk: PrecomputedChunk,
59+
): PrecomputedChunk {
60+
return chunk;
61+
}
62+
5763
export function closeWithError(destination: Destination, error: mixed): void {
5864
// $FlowFixMe: This is an Error object or the destination accepts other types.
5965
destination.destroy(error);

packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import {
3737
writeChunkAndReturn,
3838
stringToChunk,
3939
stringToPrecomputedChunk,
40+
clonePrecomputedChunk,
4041
} from 'react-server/src/ReactServerStreamConfig';
4142

4243
import {
@@ -2443,7 +2444,10 @@ export function writeCompletedBoundaryInstruction(
24432444
if (!responseState.sentCompleteBoundaryFunction) {
24442445
responseState.sentCompleteBoundaryFunction = true;
24452446
responseState.sentStyleInsertionFunction = true;
2446-
writeChunk(destination, completeBoundaryWithStylesScript1FullBoth);
2447+
writeChunk(
2448+
destination,
2449+
clonePrecomputedChunk(completeBoundaryWithStylesScript1FullBoth),
2450+
);
24472451
} else if (!responseState.sentStyleInsertionFunction) {
24482452
responseState.sentStyleInsertionFunction = true;
24492453
writeChunk(destination, completeBoundaryWithStylesScript1FullPartial);

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ const ReactNoopFlightServer = ReactFlightServer({
4545
stringToPrecomputedChunk(content: string): string {
4646
return content;
4747
},
48+
clonePrecomputedChunk(chunk: string): string {
49+
return chunk;
50+
},
4851
isModuleReference(reference: Object): boolean {
4952
return reference.$$typeof === Symbol.for('react.module.reference');
5053
},

packages/react-server-dom-relay/src/ReactServerStreamConfigFB.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,12 @@ export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
5959
return content;
6060
}
6161

62+
export function clonePrecomputedChunk(
63+
chunk: PrecomputedChunk,
64+
): PrecomputedChunk {
65+
return chunk;
66+
}
67+
6268
export function closeWithError(destination: Destination, error: mixed): void {
6369
destination.done = true;
6470
destination.fatal = true;

packages/react-server/src/ReactServerStreamConfigBrowser.js

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,15 @@ export function writeChunk(
4646
}
4747

4848
if (chunk.length > VIEW_SIZE) {
49+
if (__DEV__) {
50+
if (precomputedChunkSet.has(chunk)) {
51+
console.error(
52+
'A large precomputed chunk was passed to writeChunk without being copied.' +
53+
' Large chunks get enqueued directly and are not copied. This is incompatible with precomputed chunks because you cannot enqueue the same precomputed chunk twice.' +
54+
' Use "cloneChunk" to make a copy of this large precomputed chunk before writing it. This is a bug in React.',
55+
);
56+
}
57+
}
4958
// this chunk may overflow a single view which implies it was not
5059
// one that is cached by the streaming renderer. We will enqueu
5160
// it directly and expect it is not re-used
@@ -117,8 +126,24 @@ export function stringToChunk(content: string): Chunk {
117126
return textEncoder.encode(content);
118127
}
119128

129+
const precomputedChunkSet: Set<Chunk> = __DEV__ ? new Set() : (null: any);
130+
120131
export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
121-
return textEncoder.encode(content);
132+
const precomputedChunk = textEncoder.encode(content);
133+
134+
if (__DEV__) {
135+
precomputedChunkSet.add(precomputedChunk);
136+
}
137+
138+
return precomputedChunk;
139+
}
140+
141+
export function clonePrecomputedChunk(
142+
precomputedChunk: PrecomputedChunk,
143+
): PrecomputedChunk {
144+
return precomputedChunk.length > VIEW_SIZE
145+
? precomputedChunk.slice()
146+
: precomputedChunk;
122147
}
123148

124149
export function closeWithError(destination: Destination, error: mixed): void {

packages/react-server/src/ReactServerStreamConfigBun.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,12 @@ export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
6464
return content;
6565
}
6666

67+
export function clonePrecomputedChunk(
68+
chunk: PrecomputedChunk,
69+
): PrecomputedChunk {
70+
return chunk;
71+
}
72+
6773
export function closeWithError(destination: Destination, error: mixed): void {
6874
// $FlowFixMe[method-unbinding]
6975
if (typeof destination.error === 'function') {

packages/react-server/src/ReactServerStreamConfigNode.js

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,15 @@ function writeViewChunk(destination: Destination, chunk: PrecomputedChunk) {
9595
return;
9696
}
9797
if (chunk.byteLength > VIEW_SIZE) {
98+
if (__DEV__) {
99+
if (precomputedChunkSet && precomputedChunkSet.has(chunk)) {
100+
console.error(
101+
'A large precomputed chunk was passed to writeChunk without being copied.' +
102+
' Large chunks get enqueued directly and are not copied. This is incompatible with precomputed chunks because you cannot enqueue the same precomputed chunk twice.' +
103+
' Use "cloneChunk" to make a copy of this large precomputed chunk before writing it. This is a bug in React.',
104+
);
105+
}
106+
}
98107
// this chunk may overflow a single view which implies it was not
99108
// one that is cached by the streaming renderer. We will enqueu
100109
// it directly and expect it is not re-used
@@ -185,8 +194,26 @@ export function stringToChunk(content: string): Chunk {
185194
return content;
186195
}
187196

197+
const precomputedChunkSet = __DEV__ ? new Set() : null;
198+
188199
export function stringToPrecomputedChunk(content: string): PrecomputedChunk {
189-
return textEncoder.encode(content);
200+
const precomputedChunk = textEncoder.encode(content);
201+
202+
if (__DEV__) {
203+
if (precomputedChunkSet) {
204+
precomputedChunkSet.add(precomputedChunk);
205+
}
206+
}
207+
208+
return precomputedChunk;
209+
}
210+
211+
export function clonePrecomputedChunk(
212+
precomputedChunk: PrecomputedChunk,
213+
): PrecomputedChunk {
214+
return precomputedChunk.length > VIEW_SIZE
215+
? precomputedChunk.slice()
216+
: precomputedChunk;
190217
}
191218

192219
export function closeWithError(destination: Destination, error: mixed): void {

packages/react-server/src/forks/ReactServerStreamConfig.custom.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,3 +41,4 @@ export const close = $$$hostConfig.close;
4141
export const closeWithError = $$$hostConfig.closeWithError;
4242
export const stringToChunk = $$$hostConfig.stringToChunk;
4343
export const stringToPrecomputedChunk = $$$hostConfig.stringToPrecomputedChunk;
44+
export const clonePrecomputedChunk = $$$hostConfig.clonePrecomputedChunk;

0 commit comments

Comments
 (0)