From 8d0d0e9a8aadc4bdddff3a40871dbc54c63264f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 24 Feb 2022 20:09:03 -0500 Subject: [PATCH] Deprecate renderToNodeStream (and fix textarea bug) (#23359) * Deprecate renderToNodeStream * 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. * 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. * 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 +++++++++++-------- ...eactDOMServerIntegrationNewContext-test.js | 46 +++++++++++---- .../ReactDOMServerIntegrationTextarea-test.js | 6 ++ .../__tests__/ReactServerRendering-test.js | 16 +++++- .../ReactDOMServerIntegrationTestUtils.js | 14 ++++- .../src/server/ReactDOMLegacyServerNode.js | 5 ++ .../src/server/ReactDOMServerFormatConfig.js | 12 +++- 7 files changed, 113 insertions(+), 43 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js index a856bd42edc1d..6a777f3f4325e 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', () => { TextMore Text , ); - 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(Text); 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(
{''}foo
); - 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(
foo{''}
); - 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 , ); - 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(
{null}foo
); - 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(
{false}foo
); - 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} , ); - 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', () => { , ); 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(
); // 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], 'Text1"'); expectTextNode(e.childNodes[2], 'Text2"'); } 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'); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index 85c214067531b..4d1016dfdaced 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', () => { ); - 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__/ReactDOMServerIntegrationTextarea-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationTextarea-test.js index 261b6c041495a..1d8b388c4b3a8 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(