@samitouri / QOS-React / commits / c8a035036d

[Fizz] hoistables should never flush before the preamble (#28802)

Hoistables should never flush before the preamble however there is a surprisingly easy way to trigger this to happen by suspending in the shell of the app. This change modifies the flushing behavior to not emit any hoistables before the preamble has written. It accomplishes this by aborting the flush early if there are any pending root tasks remaining. It's unfortunate we need this extra condition but it's essential that we don't emit anything before the preamble and at the moment I don't see a way to do that without introducing a new condition. There is a test that began to fail with this update. It turns out that in node the root can be blocked during a resume even for a component inside a Suspense boundary if that boundary was part of the prerender. This means that with the current heuristic in this PR boundaries cannot be flushed during resume until the root is unblocked. This is not ideal but this is already how Edge works because the root blocks the stream in that case. This just makes Node deopt in a similar way to edge. We should improve this but we ought to do so in a way that works for edge too and it needs to be more comprehensive.

Josh Story committed Apr 11, 2024 at 15:13 UTC c8a035036d0f257c514b3628e927dd9dd26e5a09

3 files changed +64 -9

packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js

+8 -1

@@ -6926,7 +6926,14 @@ describe('ReactDOMFizzServer', () => {

6926 expect(getVisibleChildren(container)).toEqual(

6927 <div>

6928 {'Loading1'}

6929 - {'Hello'}

6929 + {/*

6930 + This used to show "Hello" in this slot because the boundary was able to be flushed

6931 + early but we now prevent flushing while pendingRootTasks is not zero. This is how Edge

6932 + would work anyway because you don't get the stream until the root is unblocked on a resume

6933 + so Node now aligns with edge bevavior

6934 + {'Hello'}

6935 + */}

6936 + {'Loading2'}

6937 {'Loading3'}

6938 </div>,

6939 );

packages/react-dom/src/__tests__/ReactDOMFloat-test.js

+45

@@ -5034,6 +5034,51 @@ body {

5034 );

5035 });

5036

5037 + it('should never flush hoistables before the preamble', async () => {

5038 + let resolve;

5039 + const promise = new Promise(res => {

5040 + resolve = res;

5041 + });

5042 +

5043 + function App() {

5044 + ReactDOM.preinit('foo', {as: 'script'});

5045 + React.use(promise);

5046 + return (

5047 + <html>

5048 + <body>hello</body>

5049 + </html>

5050 + );

5051 + }

5052 +

5053 + await act(() => {

5054 + renderToPipeableStream(<App />).pipe(writable);

5055 + });

5056 +

5057 + // we assert the default JSDOM still in tact

5058 + expect(getMeaningfulChildren(document)).toEqual(

5059 + <html>

5060 + <head />

5061 + <body>

5062 + <div id="container" />

5063 + </body>

5064 + </html>,

5065 + );

5066 +

5067 + await act(() => {

5068 + resolve();

5069 + });

5070 +

5071 + // we assert the DOM was replaced entirely because we streamed an opening html tag

5072 + expect(getMeaningfulChildren(document)).toEqual(

5073 + <html>

5074 + <head>

5075 + <script async="" src="foo" />

5076 + </head>

5077 + <body>hello</body>

5078 + </html>,

5079 + );

5080 + });

5081 +

5082 describe('ReactDOM.prefetchDNS(href)', () => {

5083 it('creates a dns-prefetch resource when called', async () => {

5084 function App({url}) {

packages/react-server/src/ReactFizzServer.js

+11 -8

@@ -4057,22 +4057,25 @@ function flushCompletedQueues(

4057 // that item fully and then yield. At that point we remove the already completed

4058 // items up until the point we completed them.

4059

4060 + if (request.pendingRootTasks > 0) {

4061 + // When there are pending root tasks we don't want to flush anything

4062 + return;

4063 + }

4064 +

4065 let i;

4066 const completedRootSegment = request.completedRootSegment;

4067 if (completedRootSegment !== null) {

4068 if (completedRootSegment.status === POSTPONED) {

4069 // We postponed the root, so we write nothing.

4070 return;

4066 - } else if (request.pendingRootTasks === 0) {

4067 - flushPreamble(request, destination, completedRootSegment);

4068 - flushSegment(request, destination, completedRootSegment, null);

4069 - request.completedRootSegment = null;

4070 - writeCompletedRoot(destination, request.renderState);

4071 - } else {

4072 - // We haven't flushed the root yet so we don't need to check any other branches further down

4073 - return;

4071 }

4072 +

4073 + flushPreamble(request, destination, completedRootSegment);

4074 + flushSegment(request, destination, completedRootSegment, null);

4075 + request.completedRootSegment = null;

4076 + writeCompletedRoot(destination, request.renderState);

4077 }

4078 +

4079 writeHoistables(destination, request.resumableState, request.renderState);

4080 // We emit client rendering instructions for already emitted boundaries first.

4081 // This is so that we can signal to the client to start client rendering them as