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 <sebastian@calyptus.eu>
1 parent 799ee65 commit 2655c93

8 files changed

Lines changed: 81 additions & 3 deletions

File tree

‎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
returncontent;
5555
}
5656

57+
exportfunctionclonePrecomputedChunk(
58+
chunk: PrecomputedChunk,
59+
): PrecomputedChunk{
60+
returnchunk;
61+
}
62+
5763
exportfunctioncloseWithError(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
}elseif(!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
returnreference.$$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
returncontent;
6060
}
6161

62+
exportfunctionclonePrecomputedChunk(
63+
chunk: PrecomputedChunk,
64+
): PrecomputedChunk{
65+
returnchunk;
66+
}
67+
6268
exportfunctioncloseWithError(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
returntextEncoder.encode(content);
118127
}
119128

129+
constprecomputedChunkSet: Set<Chunk>=__DEV__ ? newSet() : (null: any);
130+
120131
exportfunctionstringToPrecomputedChunk(content: string): PrecomputedChunk{
121-
returntextEncoder.encode(content);
132+
constprecomputedChunk=textEncoder.encode(content);
133+
134+
if(__DEV__){
135+
precomputedChunkSet.add(precomputedChunk);
136+
}
137+
138+
returnprecomputedChunk;
139+
}
140+
141+
exportfunctionclonePrecomputedChunk(
142+
precomputedChunk: PrecomputedChunk,
143+
): PrecomputedChunk{
144+
returnprecomputedChunk.length>VIEW_SIZE
145+
? precomputedChunk.slice()
146+
: precomputedChunk;
122147
}
123148

124149
exportfunctioncloseWithError(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
returncontent;
6565
}
6666

67+
exportfunctionclonePrecomputedChunk(
68+
chunk: PrecomputedChunk,
69+
): PrecomputedChunk{
70+
returnchunk;
71+
}
72+
6773
exportfunctioncloseWithError(destination: Destination,error: mixed): void{
6874
// $FlowFixMe[method-unbinding]
6975
if(typeofdestination.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
returncontent;
186195
}
187196

197+
constprecomputedChunkSet=__DEV__ ? newSet() : null;
198+
188199
exportfunctionstringToPrecomputedChunk(content: string): PrecomputedChunk{
189-
returntextEncoder.encode(content);
200+
constprecomputedChunk=textEncoder.encode(content);
201+
202+
if(__DEV__){
203+
if(precomputedChunkSet){
204+
precomputedChunkSet.add(precomputedChunk);
205+
}
206+
}
207+
208+
returnprecomputedChunk;
209+
}
210+
211+
exportfunctionclonePrecomputedChunk(
212+
precomputedChunk: PrecomputedChunk,
213+
): PrecomputedChunk{
214+
returnprecomputedChunk.length>VIEW_SIZE
215+
? precomputedChunk.slice()
216+
: precomputedChunk;
190217
}
191218

192219
exportfunctioncloseWithError(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
exportconstcloseWithError=$$$hostConfig.closeWithError;
4242
exportconststringToChunk=$$$hostConfig.stringToChunk;
4343
exportconststringToPrecomputedChunk=$$$hostConfig.stringToPrecomputedChunk;
44+
exportconstclonePrecomputedChunk=$$$hostConfig.clonePrecomputedChunk;

0 commit comments

Comments
 (0)