From 0850c4ccf3e30911b3fb038f74cbbb0069a714b4 Mon Sep 17 00:00:00 2001
From: Sebastian Markbage <sebastian@calyptus.eu>
Date: Thu, 24 Feb 2022 15:49:48 -0500
Subject: [PATCH 1/4] Deprecate renderToNodeStream

---
 packages/react-dom/src/server/ReactDOMLegacyServerNode.js | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/packages/react-dom/src/server/ReactDOMLegacyServerNode.js b/packages/react-dom/src/server/ReactDOMLegacyServerNode.js
index 71ebab5ac0f..fd55b2b0a5c 100644
--- a/packages/react-dom/src/server/ReactDOMLegacyServerNode.js
+++ b/packages/react-dom/src/server/ReactDOMLegacyServerNode.js
@@ -93,6 +93,11 @@ function renderToNodeStream(
   children: ReactNodeList,
   options?: ServerOptions,
 ): Readable {
+  if (__DEV__) {
+    console.error(
+      'renderToNodeStream is deprecated. Use renderToPipeableStream instead.',
+    );
+  }
   return renderToNodeStreamImpl(children, options, false);
 }
 

From f62de1d8e3cc24a27f36147fe7a45d0d00695745 Mon Sep 17 00:00:00 2001
From: Sebastian Markbage <sebastian@calyptus.eu>
Date: Thu, 24 Feb 2022 15:52:38 -0500
Subject: [PATCH 2/4] Use renderToPipeableStream in tests instead of
 renderToNodeStream

This is the equivalent API. This means that we have way less test coverage
of this API but I feel like that's fine since it has a deprecation warning
in it and we have coverage on renderToString that is mostly the same.
---
 ...eactDOMServerIntegrationNewContext-test.js | 46 ++++++++++++++-----
 .../__tests__/ReactServerRendering-test.js    | 16 ++++++-
 .../ReactDOMServerIntegrationTestUtils.js     | 14 ++++--
 3 files changed, 59 insertions(+), 17 deletions(-)

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js
index 85c21406753..4d1016dfdac 100644
--- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js
@@ -409,12 +409,24 @@ describe('ReactDOMServerIntegration', () => {
         </LoggedInUser.Provider>
       );
 
-      const streamAmy = ReactDOMServer.renderToNodeStream(
-        AppWithUser('Amy'),
-      ).setEncoding('utf8');
-      const streamBob = ReactDOMServer.renderToNodeStream(
-        AppWithUser('Bob'),
-      ).setEncoding('utf8');
+      let streamAmy;
+      let streamBob;
+      expect(() => {
+        streamAmy = ReactDOMServer.renderToNodeStream(
+          AppWithUser('Amy'),
+        ).setEncoding('utf8');
+      }).toErrorDev(
+        'renderToNodeStream is deprecated. Use renderToPipeableStream instead.',
+        {withoutStack: true},
+      );
+      expect(() => {
+        streamBob = ReactDOMServer.renderToNodeStream(
+          AppWithUser('Bob'),
+        ).setEncoding('utf8');
+      }).toErrorDev(
+        'renderToNodeStream is deprecated. Use renderToPipeableStream instead.',
+        {withoutStack: true},
+      );
 
       // Testing by filling the buffer using internal _read() with a small
       // number of bytes to avoid a test case which needs to align to a
@@ -449,9 +461,14 @@ describe('ReactDOMServerIntegration', () => {
       const streamCount = 34;
 
       for (let i = 0; i < streamCount; i++) {
-        streams[i] = ReactDOMServer.renderToNodeStream(
-          NthRender(i % 2 === 0 ? 'Expected to be recreated' : i),
-        ).setEncoding('utf8');
+        expect(() => {
+          streams[i] = ReactDOMServer.renderToNodeStream(
+            NthRender(i % 2 === 0 ? 'Expected to be recreated' : i),
+          ).setEncoding('utf8');
+        }).toErrorDev(
+          'renderToNodeStream is deprecated. Use renderToPipeableStream instead.',
+          {withoutStack: true},
+        );
       }
 
       // Testing by filling the buffer using internal _read() with a small
@@ -468,9 +485,14 @@ describe('ReactDOMServerIntegration', () => {
 
       // Recreate those same streams.
       for (let i = 0; i < streamCount; i += 2) {
-        streams[i] = ReactDOMServer.renderToNodeStream(
-          NthRender(i),
-        ).setEncoding('utf8');
+        expect(() => {
+          streams[i] = ReactDOMServer.renderToNodeStream(
+            NthRender(i),
+          ).setEncoding('utf8');
+        }).toErrorDev(
+          'renderToNodeStream is deprecated. Use renderToPipeableStream instead.',
+          {withoutStack: true},
+        );
       }
 
       // Read a bit from all streams again.
diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js
index 6489151b055..0c057c5fade 100644
--- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js
+++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js
@@ -569,7 +569,13 @@ describe('ReactDOMServer', () => {
   describe('renderToNodeStream', () => {
     it('should generate simple markup', () => {
       const SuccessfulElement = React.createElement(() => <img />);
-      const response = ReactDOMServer.renderToNodeStream(SuccessfulElement);
+      let response;
+      expect(() => {
+        response = ReactDOMServer.renderToNodeStream(SuccessfulElement);
+      }).toErrorDev(
+        'renderToNodeStream is deprecated. Use renderToPipeableStream instead.',
+        {withoutStack: true},
+      );
       expect(response.read().toString()).toMatch(new RegExp('<img' + '/>'));
     });
 
@@ -577,7 +583,13 @@ describe('ReactDOMServer', () => {
       const FailingElement = React.createElement(() => {
         throw new Error('An Error');
       });
-      const response = ReactDOMServer.renderToNodeStream(FailingElement);
+      let response;
+      expect(() => {
+        response = ReactDOMServer.renderToNodeStream(FailingElement);
+      }).toErrorDev(
+        'renderToNodeStream is deprecated. Use renderToPipeableStream instead.',
+        {withoutStack: true},
+      );
       return new Promise(resolve => {
         response.once('error', () => {
           resolve();
diff --git a/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js b/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js
index 8714059bb38..9f23d7aa96a 100644
--- a/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js
+++ b/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js
@@ -154,8 +154,11 @@ module.exports = function(initModules) {
       () =>
         new Promise((resolve, reject) => {
           const writable = new DrainWritable();
-          const s = ReactDOMServer.renderToNodeStream(reactElement);
-          s.on('error', e => reject(e));
+          const s = ReactDOMServer.renderToPipeableStream(reactElement, {
+            onErrorShell(e) {
+              reject(e);
+            },
+          });
           s.pipe(writable);
           writable.on('finish', () => resolve(writable.buffer));
         }),
@@ -168,7 +171,12 @@ module.exports = function(initModules) {
   // Does not render on client or perform client-side revival.
   async function streamRender(reactElement, errorCount = 0) {
     const markup = await renderIntoStream(reactElement, errorCount);
-    return getContainerFromMarkup(reactElement, markup).firstChild;
+    let firstNode = getContainerFromMarkup(reactElement, markup).firstChild;
+    if (firstNode && firstNode.nodeType === Node.DOCUMENT_TYPE_NODE) {
+      // Skip document type nodes.
+      firstNode = firstNode.nextSibling;
+    }
+    return firstNode;
   }
 
   const clientCleanRender = (element, errorCount = 0) => {

From e915eee9fcc56cfbfe42f7e4b50d6cf3b9db9c6b Mon Sep 17 00:00:00 2001
From: Sebastian Markbage <sebastian@calyptus.eu>
Date: Thu, 24 Feb 2022 17:49:12 -0500
Subject: [PATCH 3/4] Fix textarea bug

The test changes revealed a bug with textarea. It happens because we
currently always insert trailing comment nodes. We should optimize that
away. However, we also don't really support complex children so we
should toString it anyway which is what partial renderer used to do.
---
 .../ReactDOMServerIntegrationTextarea-test.js        |  6 ++++++
 .../src/server/ReactDOMServerFormatConfig.js         | 12 +++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationTextarea-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationTextarea-test.js
index 261b6c04149..1d8b388c4b3 100644
--- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationTextarea-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationTextarea-test.js
@@ -49,6 +49,12 @@ describe('ReactDOMServerIntegrationTextarea', () => {
     expect(e.value).toBe('foo');
   });
 
+  itRenders('a textarea with a value of undefined', async render => {
+    const e = await render(<textarea value={undefined} />);
+    expect(e.getAttribute('value')).toBe(null);
+    expect(e.value).toBe('');
+  });
+
   itRenders('a textarea with a value and readOnly', async render => {
     const e = await render(<textarea value="foo" readOnly={true} />);
     // textarea DOM elements don't have a value **attribute**, the text is
diff --git a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
index 20887c22601..abbcedda22d 100644
--- a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
+++ b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js
@@ -1001,7 +1001,17 @@ function pushStartTextArea(
     target.push(leadingNewline);
   }
 
-  return value;
+  // ToString and push directly instead of recurse over children.
+  // We don't really support complex children in the value anyway.
+  // This also currently avoids a trailing comment node which breaks textarea.
+  if (value !== null) {
+    if (__DEV__) {
+      checkAttributeStringCoercion(value, 'value');
+    }
+    target.push(stringToChunk(encodeHTMLTextNode('' + value)));
+  }
+
+  return null;
 }
 
 function pushSelfClosing(

From 509678d15b79702aff0e7ece484dfdce549828cf Mon Sep 17 00:00:00 2001
From: Sebastian Markbage <sebastian@calyptus.eu>
Date: Thu, 24 Feb 2022 18:14:36 -0500
Subject: [PATCH 4/4] Update tests that assert number of nodes

These tests are unnecessarily specific about number of nodes.

I special case these, which these tests already do, because they're good
tests to test that the optimization actually works later when we do
fix it.
---
 .../ReactDOMServerIntegrationElements-test.js | 57 +++++++++++--------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js
index a856bd42edc..6a777f3f432 100644
--- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js
@@ -101,7 +101,7 @@ describe('ReactDOMServerIntegration', () => {
         ) {
           // For plain server markup result we have comments between.
           // If we're able to hydrate, they remain.
-          expect(e.childNodes.length).toBe(5);
+          expect(e.childNodes.length).toBe(render === streamRender ? 6 : 5);
           expectTextNode(e.childNodes[0], ' ');
           expectTextNode(e.childNodes[2], ' ');
           expectTextNode(e.childNodes[4], ' ');
@@ -119,8 +119,8 @@ describe('ReactDOMServerIntegration', () => {
             Text<span>More Text</span>
           </div>,
         );
-        expect(e.childNodes.length).toBe(2);
-        const spanNode = e.childNodes[1];
+        expect(e.childNodes.length).toBe(render === streamRender ? 3 : 2);
+        const spanNode = e.childNodes[render === streamRender ? 2 : 1];
         expectTextNode(e.childNodes[0], 'Text');
         expect(spanNode.tagName).toBe('SPAN');
         expect(spanNode.childNodes.length).toBe(1);
@@ -147,19 +147,19 @@ describe('ReactDOMServerIntegration', () => {
       itRenders('a custom element with text', async render => {
         const e = await render(<custom-element>Text</custom-element>);
         expect(e.tagName).toBe('CUSTOM-ELEMENT');
-        expect(e.childNodes.length).toBe(1);
+        expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
         expectNode(e.firstChild, TEXT_NODE_TYPE, 'Text');
       });
 
       itRenders('a leading blank child with a text sibling', async render => {
         const e = await render(<div>{''}foo</div>);
-        expect(e.childNodes.length).toBe(1);
+        expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
         expectTextNode(e.childNodes[0], 'foo');
       });
 
       itRenders('a trailing blank child with a text sibling', async render => {
         const e = await render(<div>foo{''}</div>);
-        expect(e.childNodes.length).toBe(1);
+        expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
         expectTextNode(e.childNodes[0], 'foo');
       });
 
@@ -176,7 +176,7 @@ describe('ReactDOMServerIntegration', () => {
           render === streamRender
         ) {
           // In the server render output there's a comment between them.
-          expect(e.childNodes.length).toBe(3);
+          expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
           expectTextNode(e.childNodes[0], 'foo');
           expectTextNode(e.childNodes[2], 'bar');
         } else {
@@ -203,7 +203,7 @@ describe('ReactDOMServerIntegration', () => {
             render === streamRender
           ) {
             // In the server render output there's a comment between them.
-            expect(e.childNodes.length).toBe(5);
+            expect(e.childNodes.length).toBe(render === streamRender ? 6 : 5);
             expectTextNode(e.childNodes[0], 'a');
             expectTextNode(e.childNodes[2], 'b');
             expectTextNode(e.childNodes[4], 'c');
@@ -240,11 +240,7 @@ describe('ReactDOMServerIntegration', () => {
             e
           </div>,
         );
-        if (
-          render === serverRender ||
-          render === clientRenderOnServerString ||
-          render === streamRender
-        ) {
+        if (render === serverRender || render === clientRenderOnServerString) {
           // In the server render output there's comments between text nodes.
           expect(e.childNodes.length).toBe(5);
           expectTextNode(e.childNodes[0], 'a');
@@ -253,6 +249,15 @@ describe('ReactDOMServerIntegration', () => {
           expectTextNode(e.childNodes[3].childNodes[0], 'c');
           expectTextNode(e.childNodes[3].childNodes[2], 'd');
           expectTextNode(e.childNodes[4], 'e');
+        } else if (render === streamRender) {
+          // In the server render output there's comments after each text node.
+          expect(e.childNodes.length).toBe(7);
+          expectTextNode(e.childNodes[0], 'a');
+          expectTextNode(e.childNodes[2], 'b');
+          expect(e.childNodes[4].childNodes.length).toBe(4);
+          expectTextNode(e.childNodes[4].childNodes[0], 'c');
+          expectTextNode(e.childNodes[4].childNodes[2], 'd');
+          expectTextNode(e.childNodes[5], 'e');
         } else {
           expect(e.childNodes.length).toBe(4);
           expectTextNode(e.childNodes[0], 'a');
@@ -291,7 +296,7 @@ describe('ReactDOMServerIntegration', () => {
           render === streamRender
         ) {
           // In the server markup there's a comment between.
-          expect(e.childNodes.length).toBe(3);
+          expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
           expectTextNode(e.childNodes[0], 'foo');
           expectTextNode(e.childNodes[2], '40');
         } else {
@@ -330,13 +335,13 @@ describe('ReactDOMServerIntegration', () => {
 
       itRenders('null children as blank', async render => {
         const e = await render(<div>{null}foo</div>);
-        expect(e.childNodes.length).toBe(1);
+        expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
         expectTextNode(e.childNodes[0], 'foo');
       });
 
       itRenders('false children as blank', async render => {
         const e = await render(<div>{false}foo</div>);
-        expect(e.childNodes.length).toBe(1);
+        expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
         expectTextNode(e.childNodes[0], 'foo');
       });
 
@@ -348,7 +353,7 @@ describe('ReactDOMServerIntegration', () => {
             {false}
           </div>,
         );
-        expect(e.childNodes.length).toBe(1);
+        expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1);
         expectTextNode(e.childNodes[0], 'foo');
       });
 
@@ -735,10 +740,10 @@ describe('ReactDOMServerIntegration', () => {
             </div>,
           );
           expect(e.id).toBe('parent');
-          expect(e.childNodes.length).toBe(3);
+          expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
           const child1 = e.childNodes[0];
           const textNode = e.childNodes[1];
-          const child2 = e.childNodes[2];
+          const child2 = e.childNodes[render === streamRender ? 3 : 2];
           expect(child1.id).toBe('child1');
           expect(child1.childNodes.length).toBe(0);
           expectTextNode(textNode, ' ');
@@ -752,10 +757,10 @@ describe('ReactDOMServerIntegration', () => {
         async render => {
           // prettier-ignore
           const e = await render(<div id="parent">  <div id="child" />   </div>); // eslint-disable-line no-multi-spaces
-          expect(e.childNodes.length).toBe(3);
+          expect(e.childNodes.length).toBe(render === streamRender ? 5 : 3);
           const textNode1 = e.childNodes[0];
-          const child = e.childNodes[1];
-          const textNode2 = e.childNodes[2];
+          const child = e.childNodes[render === streamRender ? 2 : 1];
+          const textNode2 = e.childNodes[render === streamRender ? 3 : 2];
           expect(e.id).toBe('parent');
           expectTextNode(textNode1, '  ');
           expect(child.id).toBe('child');
@@ -778,7 +783,9 @@ describe('ReactDOMServerIntegration', () => {
         ) {
           // For plain server markup result we have comments between.
           // If we're able to hydrate, they remain.
-          expect(parent.childNodes.length).toBe(5);
+          expect(parent.childNodes.length).toBe(
+            render === streamRender ? 6 : 5,
+          );
           expectTextNode(parent.childNodes[0], 'a');
           expectTextNode(parent.childNodes[2], 'b');
           expectTextNode(parent.childNodes[4], 'c');
@@ -810,7 +817,7 @@ describe('ReactDOMServerIntegration', () => {
           render === clientRenderOnServerString ||
           render === streamRender
         ) {
-          expect(e.childNodes.length).toBe(3);
+          expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
           expectTextNode(e.childNodes[0], '<span>Text1&quot;</span>');
           expectTextNode(e.childNodes[2], '<span>Text2&quot;</span>');
         } else {
@@ -861,7 +868,7 @@ describe('ReactDOMServerIntegration', () => {
           );
           if (render === serverRender || render === streamRender) {
             // We have three nodes because there is a comment between them.
-            expect(e.childNodes.length).toBe(3);
+            expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3);
             // Everything becomes LF when parsed from server HTML.
             // Null character is ignored.
             expectNode(e.childNodes[0], TEXT_NODE_TYPE, 'foo\nbar');