From 96fad9f90f0ed58eaed13dab64ddc9dd24db355d Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 11:26:39 -0700 Subject: [PATCH 01/10] [Fizz] Improve text separator byte efficiency Previously text separators were inserted following any Text node in Fizz. This increases bytes sent when streaming and in some cases such as title elements these separators are not interpretted as comment nodes and leak into the visual aspects of a page as escaped text. The reason simple tracking on the last pushed type doesn't work is that Segments can be filled in asynchronously later and so you cannot know in a single pass whether the preceding content was a text node or not. This commit adds a concetp of TextEmbedding which provides a best effort signal to Segments on whether they are embedded within text. This allows the later resolution of that Segment to add text separators when possibly necessary but avoid them when they are surely not. The current implementation can only "peek" head if the segment is a the Root Segment or a Suspense Boundary Segment. In these cases we know there is no trailing text embedding and we can eliminate the separator at the end of the segment if the last emitted element was Text. In normal Segments we cannot peek and thus have to assume there might be a trailing text embedding and we issue a separator defensively. This should be rare in practice as it is assumed most components that will cause segment creation will also emit some markup at the edges. --- .../src/__tests__/ReactDOMFizzServer-test.js | 221 ++++++++++++++++++ .../src/server/ReactDOMServerFormatConfig.js | 64 ++++- .../ReactDOMServerLegacyFormatConfig.js | 4 + .../server/ReactNativeServerFormatConfig.js | 9 + .../src/ReactNoopServer.js | 9 + packages/react-server/src/ReactFizzServer.js | 29 ++- .../forks/ReactServerFormatConfig.custom.js | 5 + 7 files changed, 338 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 9a6cfd457c8d..86b88060a79c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -234,6 +234,11 @@ describe('ReactDOMFizzServer', () => { return readText(text); } + function AsyncTextWrapped({as, text}) { + let As = as; + return {readText(text)}; + } + // @gate experimental it('should asynchronously load a lazy component', async () => { let resolveA; @@ -3577,4 +3582,220 @@ describe('ReactDOMFizzServer', () => { , ); }); + + describe('text separators', () => { + it('it only includes separators between adjacent text nodes', async () => { + function App({name}) { + return ( +
+ helloworld, {name}! +
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream( + , + ); + pipe(writable); + }); + + expect(container.innerHTML).toEqual( + '
helloworld, Foo!
', + ); + }); + + it('it inserts text separators even when adjacent text is in a delayed segment', async () => { + function App({name}) { + return ( + +
+ hello + + world, + + ! +
+
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream( + , + ); + pipe(writable); + }); + + expect(document.getElementById('app-div').outerHTML).toEqual( + '
helloworld, !
', + ); + + await act(() => resolveText('Foo')); + + expect(container.firstElementChild.outerHTML).toEqual( + '
helloworld, Foo!
', + ); + }); + + it('it works with multiple adjacent segments', async () => { + function App() { + return ( + +
+ h + w +
+
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + pipe(writable); + }); + + expect(document.getElementById('app-div').outerHTML).toEqual( + '
hw
', + ); + + await act(() => resolveText('orld')); + + expect(document.getElementById('app-div').outerHTML).toEqual( + '
hworld
', + ); + + await act(() => resolveText('ello')); + expect(container.firstElementChild.outerHTML).toEqual( + '
helloworld
', + ); + }); + + it('it works when some segments are flushed and others are patched', async () => { + function App() { + return ( + +
+ h + w +
+
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await act(() => resolveText('ello')); + pipe(writable); + }); + + expect(document.getElementById('app-div').outerHTML).toEqual( + '
hellow
', + ); + + await act(() => resolveText('orld')); + + expect(container.firstElementChild.outerHTML).toEqual( + '
helloworld
', + ); + }); + + it('it does not prepend a text separators if the segment follows a non-Text Node', async () => { + function App() { + return ( + +
+ hello + + + +
+
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await act(() => resolveText('world')); + pipe(writable); + }); + + expect(container.firstElementChild.outerHTML).toEqual( + '
helloworld
', + ); + }); + + it('it does not prepend a text separators if the segments first emission is a non-Text Node', async () => { + function App() { + return ( + +
+ hello + +
+
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await act(() => resolveText('world')); + pipe(writable); + }); + + expect(container.firstElementChild.outerHTML).toEqual( + '
helloworld
', + ); + }); + + it('should not insert separators for text inside Suspense boundaries even if they would otherwise be considered text-embedded', async () => { + function App() { + return ( + +
+ start + + firststart + + firstend + + + secondstart + + + + + end +
+
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await act(() => resolveText('world')); + pipe(writable); + }); + + expect(document.getElementById('app-div').outerHTML).toEqual( + '
start[loading first][loading second]end
', + ); + + await act(async () => { + resolveText('first suspended'); + }); + + expect(document.getElementById('app-div').outerHTML).toEqual( + '
startfirststartfirst suspendedfirstend[loading second]end
', + ); + + await act(async () => { + resolveText('second suspended'); + }); + + expect(container.firstElementChild.outerHTML).toEqual( + '
startfirststartfirst suspendedfirstendsecondstartsecond suspendedend
', + ); + }); + }); }); diff --git a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js index c8d6d35ceb6b..44f083a41145 100644 --- a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js @@ -279,6 +279,7 @@ function encodeHTMLTextNode(text: string): string { } const textSeparator = stringToPrecomputedChunk(''); +let lastPushedText = false; export function pushTextInstance( target: Array, @@ -289,8 +290,65 @@ export function pushTextInstance( // Empty text doesn't have a DOM node representation and the hydration is aware of this. return; } - // TODO: Avoid adding a text separator in common cases. - target.push(stringToChunk(encodeHTMLTextNode(text)), textSeparator); + // We use a null charater to wrap every text chunk. we use this to boundary check whether we need + // to insert a Node textSeparator like when flushing segments between sets of chunks. + if (lastPushedText) { + target.push(textSeparator); + } + lastPushedText = true; + target.push(stringToChunk(encodeHTMLTextNode(text))); +} + +// The Text Embedding flags allow the format config to know when to include or exclude +// text separators between parent and child segments. We track the leading edge and +// trailing edges separately however due to the fact that peeking ahead is usually +// not possible we discern between knowing we need a separator at the leading edge and +// assuming we might need one at the trailing edge. In the future we could get more +// advanced with the tracking and drop the trailing edge in some cases when we know a segment +// is followed by a Text or non-Text Node explicitly vs followed by another Segment +export opaque type TextEmbedding = number; +type Embeddable = {textEmbedding: TextEmbedding, ...}; + +const NO_TEXT_EMBED = /* */ 0b0000; + +const LEADING_SEPARATOR_NEEDED = /* */ 0b0011; +const LEADING_TEXT_EMBED_KNOWN = /* */ 0b0001; +// const LEADING_TEXT_EMBED_POSSIBLE = /* */ 0b0010; + +const TRAILING_SEPARATOR_NEEDED = /* */ 0b1100; +// const TRAILING_TEXT_EMBED_KNOWN = /* */ 0b0100; +const TRAILING_TEXT_EMBED_POSSIBLE = /* */ 0b1000; + +// Suspense boundaries always emit a comment node at the leading and trailing edge and thus need +// no additional separators. we also use this for the root segment even though it isn't a Boundary +// because it cannot have pre or post text and thus matches the same embedding semantics or Boundaries +export function textEmbeddingForBoundarySegment(): TextEmbedding { + lastPushedText = false; + return NO_TEXT_EMBED; +} + +export function textEmbeddingForSegment(): TextEmbedding { + let embedding = TRAILING_TEXT_EMBED_POSSIBLE; + embedding |= lastPushedText ? LEADING_TEXT_EMBED_KNOWN : NO_TEXT_EMBED; + + lastPushedText = false; + return embedding; +} + +// Called when a segment is about to be rendered by a Task +export function prepareForSegment(textEmbedding: TextEmbedding) { + lastPushedText = textEmbedding & LEADING_SEPARATOR_NEEDED; +} + +// Called when a segment is exhaustively rendered +export function finalizeForSegment( + target: Array, + textEmbedding: TextEmbedding, +) { + if (lastPushedText && textEmbedding & TRAILING_SEPARATOR_NEEDED) { + target.push(textSeparator); + } + lastPushedText = false; } const styleNameCache: Map = new Map(); @@ -1324,6 +1382,7 @@ export function pushStartInstance( responseState: ResponseState, formatContext: FormatContext, ): ReactNodeList { + lastPushedText = false; if (__DEV__) { validateARIAProperties(type, props); validateInputProperties(type, props); @@ -1436,6 +1495,7 @@ export function pushEndInstance( type: string, props: Object, ): void { + lastPushedText = false; switch (type) { // Omitted close tags // TODO: Instead of repeating this switch we could try to pass a flag from above. diff --git a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js index c3d09f481fb6..8be02f8ea625 100644 --- a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js @@ -95,6 +95,10 @@ export { writeEndPendingSuspenseBoundary, writePlaceholder, writeCompletedRoot, + textEmbeddingForBoundarySegment, + textEmbeddingForSegment, + prepareForSegment, + finalizeForSegment, } from './ReactDOMServerFormatConfig'; import {stringToChunk} from 'react-server/src/ReactServerStreamConfig'; diff --git a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js index e2b555720163..79bf325e6a63 100644 --- a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js +++ b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js @@ -156,6 +156,15 @@ export function pushEndInstance( target.push(END); } +export function textEmbeddingForBoundarySegment() { + return null; +} +export function textEmbeddingForSegment() { + return null; +} +export function prepareForSegment() {} +export function finalizeForSegment() {} + export function writeCompletedRoot( destination: Destination, responseState: ResponseState, diff --git a/packages/react-noop-renderer/src/ReactNoopServer.js b/packages/react-noop-renderer/src/ReactNoopServer.js index 5bd7e79cf724..4aa3a186d918 100644 --- a/packages/react-noop-renderer/src/ReactNoopServer.js +++ b/packages/react-noop-renderer/src/ReactNoopServer.js @@ -128,6 +128,15 @@ const ReactNoopServer = ReactFizzServer({ target.push(POP); }, + textEmbeddingForBoundarySegment() { + return null; + }, + textEmbeddingForSegment() { + return null; + }, + prepareForSegment() {}, + finalizeForSegment() {}, + writeCompletedRoot( destination: Destination, responseState: ResponseState, diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index d206a69e48c3..1e491ad3da88 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -26,6 +26,7 @@ import type { import type {ContextSnapshot} from './ReactFizzNewContext'; import type {ComponentStackNode} from './ReactFizzComponentStack'; import type {TreeContext} from './ReactFizzTreeContext'; +import type {TextEmbedding} from './ReactServerFormatConfig'; import { scheduleWork, @@ -59,6 +60,10 @@ import { UNINITIALIZED_SUSPENSE_BOUNDARY_ID, assignSuspenseBoundaryID, getChildFormatContext, + textEmbeddingForBoundarySegment, + textEmbeddingForSegment, + prepareForSegment, + finalizeForSegment, } from './ReactServerFormatConfig'; import { constructClassInstance, @@ -169,6 +174,8 @@ type Segment = { formatContext: FormatContext, // If this segment represents a fallback, this is the content that will replace that fallback. +boundary: null | SuspenseBoundary, + // opaque data that tells the formatter uses to modify output based on how the Segment is embedded in other output + +textEmbedding: TextEmbedding, }; const OPEN = 0; @@ -267,7 +274,13 @@ export function createRequest( onFatalError: onFatalError === undefined ? noop : onFatalError, }; // This segment represents the root fallback. - const rootSegment = createPendingSegment(request, 0, null, rootFormatContext); + const rootSegment = createPendingSegment( + request, + 0, + null, + rootFormatContext, + textEmbeddingForBoundarySegment(), + ); // There is no parent so conceptually, we're unblocked to flush this segment. rootSegment.parentFlushed = true; const rootTask = createTask( @@ -346,6 +359,7 @@ function createPendingSegment( index: number, boundary: null | SuspenseBoundary, formatContext: FormatContext, + textEmbedding: TextEmbedding, ): Segment { return { status: PENDING, @@ -356,6 +370,7 @@ function createPendingSegment( children: [], formatContext, boundary, + textEmbedding, }; } @@ -454,11 +469,13 @@ function renderSuspenseBoundary( const newBoundary = createSuspenseBoundary(request, fallbackAbortSet); const insertionIndex = parentSegment.chunks.length; // The children of the boundary segment is actually the fallback. + const textEmbedding = textEmbeddingForBoundarySegment(); const boundarySegment = createPendingSegment( request, insertionIndex, newBoundary, parentSegment.formatContext, + textEmbedding, ); parentSegment.children.push(boundarySegment); @@ -468,6 +485,7 @@ function renderSuspenseBoundary( 0, null, parentSegment.formatContext, + textEmbedding, ); // We mark the root segment as having its parent flushed. It's not really flushed but there is // no parent segment so there's nothing to wait on. @@ -485,7 +503,12 @@ function renderSuspenseBoundary( task.blockedSegment = contentRootSegment; try { // We use the safe form because we don't handle suspending here. Only error handling. + prepareForSegment(contentRootSegment.textEmbedding); renderNode(request, task, content); + finalizeForSegment( + contentRootSegment.chunks, + contentRootSegment.textEmbedding, + ); contentRootSegment.status = COMPLETED; queueCompletedSegment(newBoundary, contentRootSegment); if (newBoundary.pendingTasks === 0) { @@ -1263,11 +1286,13 @@ function spawnNewSuspendedTask( // Something suspended, we'll need to create a new segment and resolve it later. const segment = task.blockedSegment; const insertionIndex = segment.chunks.length; + const textEmbedding = textEmbeddingForSegment(); const newSegment = createPendingSegment( request, insertionIndex, null, segment.formatContext, + textEmbedding, ); segment.children.push(newSegment); const newTask = createTask( @@ -1544,7 +1569,9 @@ function retryTask(request: Request, task: Task): void { try { // We call the destructive form that mutates this task. That way if something // suspends again, we can reuse the same task instead of spawning a new one. + prepareForSegment(segment.textEmbedding); renderNodeDestructive(request, task, task.node); + finalizeForSegment(segment.chunks, segment.textEmbedding); task.abortSet.delete(task); segment.status = COMPLETED; diff --git a/packages/react-server/src/forks/ReactServerFormatConfig.custom.js b/packages/react-server/src/forks/ReactServerFormatConfig.custom.js index 035490fb3f65..75be9ea57845 100644 --- a/packages/react-server/src/forks/ReactServerFormatConfig.custom.js +++ b/packages/react-server/src/forks/ReactServerFormatConfig.custom.js @@ -43,6 +43,11 @@ export const pushStartCompletedSuspenseBoundary = $$$hostConfig.pushStartCompletedSuspenseBoundary; export const pushEndCompletedSuspenseBoundary = $$$hostConfig.pushEndCompletedSuspenseBoundary; +export const textEmbeddingForBoundarySegment = + $$$hostConfig.textEmbeddingForBoundarySegment; +export const textEmbeddingForSegment = $$$hostConfig.textEmbeddingForSegment; +export const prepareForSegment = $$$hostConfig.prepareForSegment; +export const finalizeForSegment = $$$hostConfig.finalizeForSegment; export const writeCompletedRoot = $$$hostConfig.writeCompletedRoot; export const writePlaceholder = $$$hostConfig.writePlaceholder; export const writeStartCompletedSuspenseBoundary = From 63934019efc8351d97c7dba4e2bafe6ced0f7b49 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 11:13:20 -0700 Subject: [PATCH 02/10] update tests that relied on older textSeparator behavior --- .../ReactDOMServerIntegrationElements-test.js | 57 ++++++++----------- .../src/__tests__/ReactDOMUseId-test.js | 2 - .../ReactDOMServerFB-test.internal.js | 4 +- 3 files changed, 26 insertions(+), 37 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js index 6a777f3f4325..0dcfbbd78c8a 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(render === streamRender ? 6 : 5); + expect(e.childNodes.length).toBe(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(render === streamRender ? 3 : 2); - const spanNode = e.childNodes[render === streamRender ? 2 : 1]; + expect(e.childNodes.length).toBe(2); + const spanNode = e.childNodes[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(render === streamRender ? 2 : 1); + expect(e.childNodes.length).toBe(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(render === streamRender ? 2 : 1); + expect(e.childNodes.length).toBe(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(render === streamRender ? 2 : 1); + expect(e.childNodes.length).toBe(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(render === streamRender ? 4 : 3); + expect(e.childNodes.length).toBe(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(render === streamRender ? 6 : 5); + expect(e.childNodes.length).toBe(5); expectTextNode(e.childNodes[0], 'a'); expectTextNode(e.childNodes[2], 'b'); expectTextNode(e.childNodes[4], 'c'); @@ -240,7 +240,11 @@ describe('ReactDOMServerIntegration', () => { e , ); - if (render === serverRender || render === clientRenderOnServerString) { + if ( + render === serverRender || + render === streamRender || + render === clientRenderOnServerString + ) { // In the server render output there's comments between text nodes. expect(e.childNodes.length).toBe(5); expectTextNode(e.childNodes[0], 'a'); @@ -249,15 +253,6 @@ 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'); @@ -296,7 +291,7 @@ describe('ReactDOMServerIntegration', () => { render === streamRender ) { // In the server markup there's a comment between. - expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3); + expect(e.childNodes.length).toBe(3); expectTextNode(e.childNodes[0], 'foo'); expectTextNode(e.childNodes[2], '40'); } else { @@ -335,13 +330,13 @@ describe('ReactDOMServerIntegration', () => { itRenders('null children as blank', async render => { const e = await render(
{null}foo
); - expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1); + expect(e.childNodes.length).toBe(1); expectTextNode(e.childNodes[0], 'foo'); }); itRenders('false children as blank', async render => { const e = await render(
{false}foo
); - expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1); + expect(e.childNodes.length).toBe(1); expectTextNode(e.childNodes[0], 'foo'); }); @@ -353,7 +348,7 @@ describe('ReactDOMServerIntegration', () => { {false} , ); - expect(e.childNodes.length).toBe(render === streamRender ? 2 : 1); + expect(e.childNodes.length).toBe(1); expectTextNode(e.childNodes[0], 'foo'); }); @@ -740,10 +735,10 @@ describe('ReactDOMServerIntegration', () => { , ); expect(e.id).toBe('parent'); - expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3); + expect(e.childNodes.length).toBe(3); const child1 = e.childNodes[0]; const textNode = e.childNodes[1]; - const child2 = e.childNodes[render === streamRender ? 3 : 2]; + const child2 = e.childNodes[2]; expect(child1.id).toBe('child1'); expect(child1.childNodes.length).toBe(0); expectTextNode(textNode, ' '); @@ -757,10 +752,10 @@ describe('ReactDOMServerIntegration', () => { async render => { // prettier-ignore const e = await render(
); // eslint-disable-line no-multi-spaces - expect(e.childNodes.length).toBe(render === streamRender ? 5 : 3); + expect(e.childNodes.length).toBe(3); const textNode1 = e.childNodes[0]; - const child = e.childNodes[render === streamRender ? 2 : 1]; - const textNode2 = e.childNodes[render === streamRender ? 3 : 2]; + const child = e.childNodes[1]; + const textNode2 = e.childNodes[2]; expect(e.id).toBe('parent'); expectTextNode(textNode1, ' '); expect(child.id).toBe('child'); @@ -783,9 +778,7 @@ describe('ReactDOMServerIntegration', () => { ) { // For plain server markup result we have comments between. // If we're able to hydrate, they remain. - expect(parent.childNodes.length).toBe( - render === streamRender ? 6 : 5, - ); + expect(parent.childNodes.length).toBe(5); expectTextNode(parent.childNodes[0], 'a'); expectTextNode(parent.childNodes[2], 'b'); expectTextNode(parent.childNodes[4], 'c'); @@ -817,7 +810,7 @@ describe('ReactDOMServerIntegration', () => { render === clientRenderOnServerString || render === streamRender ) { - expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3); + expect(e.childNodes.length).toBe(3); expectTextNode(e.childNodes[0], 'Text1"'); expectTextNode(e.childNodes[2], 'Text2"'); } else { @@ -868,7 +861,7 @@ describe('ReactDOMServerIntegration', () => { ); if (render === serverRender || render === streamRender) { // We have three nodes because there is a comment between them. - expect(e.childNodes.length).toBe(render === streamRender ? 4 : 3); + expect(e.childNodes.length).toBe(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__/ReactDOMUseId-test.js b/packages/react-dom/src/__tests__/ReactDOMUseId-test.js index 4148eeaa2e64..53d972119081 100644 --- a/packages/react-dom/src/__tests__/ReactDOMUseId-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMUseId-test.js @@ -343,7 +343,6 @@ describe('useId', () => { id="container" > :R0:, :R0H1:, :R0H2: -
`); }); @@ -369,7 +368,6 @@ describe('useId', () => { id="container" > :R0: - `); }); diff --git a/packages/react-server-dom-relay/src/__tests__/ReactDOMServerFB-test.internal.js b/packages/react-server-dom-relay/src/__tests__/ReactDOMServerFB-test.internal.js index 7444ae6f9093..8c7c6844ab96 100644 --- a/packages/react-server-dom-relay/src/__tests__/ReactDOMServerFB-test.internal.js +++ b/packages/react-server-dom-relay/src/__tests__/ReactDOMServerFB-test.internal.js @@ -93,9 +93,7 @@ describe('ReactDOMServerFB', () => { await jest.runAllTimers(); const result = readResult(stream); - expect(result).toMatchInlineSnapshot( - `"
Done
"`, - ); + expect(result).toMatchInlineSnapshot(`"
Done
"`); }); it('should throw an error when an error is thrown at the root', () => { From a9e72bf1626e9cb2bb8ad1fdfac063ecc2bf95e2 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 14:06:38 -0700 Subject: [PATCH 03/10] [Fizz] Improve separator efficiency when flushing delayed segments The method by which we get segment markup into the DOM differs depending on when the Segment resolves. If a Segment resovles before flushing begins for it's parent it will be emitted inline with the parent markup. In these cases separators may be necessary because they are how we clue the browser into breakup up text into distinct nodes that will later match up with what will be hydrated on the client. If a Segment resolves after flushing has happened a script will be used to patch up the DOM in the client. when this happens if there are any text nodes on the boundary of the patch they won't be "merged" and thus will continue to have distinct representation as Nodes in the DOM. Thus we can avoid doing any separators at the boudnaries in these cases. After applying these changes the only time you will get text separators as follows * in between serial text nodes that emmit at the same time - these are necessary and cannot be eliminated unless we stop relying on the browser to automatically parse the correct text nodes when processing this HTML * after a final text node in a non-boundary segment that resolves before it's parent has flushed - these are sometimes extraneous, like when the next emitted thing is a non-Text node. In all other cases text separators should be omitted which means the general byte efficiency of this approach should be pretty good --- .../src/__tests__/ReactDOMFizzServer-test.js | 211 +++++++++++++++++- .../src/server/ReactDOMServerFormatConfig.js | 19 +- .../ReactDOMServerLegacyFormatConfig.js | 2 + .../server/ReactNativeServerFormatConfig.js | 11 +- packages/react-server/src/ReactFizzServer.js | 4 +- .../forks/ReactServerFormatConfig.custom.js | 2 + 6 files changed, 226 insertions(+), 23 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 86b88060a79c..fd74ff41518d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -235,7 +235,7 @@ describe('ReactDOMFizzServer', () => { } function AsyncTextWrapped({as, text}) { - let As = as; + const As = as; return {readText(text)}; } @@ -3584,6 +3584,14 @@ describe('ReactDOMFizzServer', () => { }); describe('text separators', () => { + // To force performWork to start before resolving AsyncText but before piping we need to wait until + // after scheduleWork which currently uses setImmediate to delay performWork + function afterImmediate() { + return new Promise(resolve => { + setImmediate(resolve); + }); + } + it('it only includes separators between adjacent text nodes', async () => { function App({name}) { return ( @@ -3603,9 +3611,22 @@ describe('ReactDOMFizzServer', () => { expect(container.innerHTML).toEqual( '
helloworld, Foo!
', ); + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ helloworld, {'Foo'}! +
, + ); }); - it('it inserts text separators even when adjacent text is in a delayed segment', async () => { + it('it does not insert text separators even when adjacent text is in a delayed segment', async () => { function App({name}) { return ( @@ -3634,7 +3655,29 @@ describe('ReactDOMFizzServer', () => { await act(() => resolveText('Foo')); expect(container.firstElementChild.outerHTML).toEqual( - '
helloworld, Foo!
', + '
helloworld, Foo!
', + ); + // there are extra script nodes at the end of container + expect(container.childNodes.length).toBe(5); + const div = container.childNodes[1]; + expect(div.childNodes.length).toBe(3); + const b = div.childNodes[1]; + expect(b.childNodes.length).toBe(2); + expect(b.childNodes[0]).toMatchInlineSnapshot('world, '); + expect(b.childNodes[1]).toMatchInlineSnapshot('Foo'); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ helloworld, {'Foo'}! +
, ); }); @@ -3662,12 +3705,24 @@ describe('ReactDOMFizzServer', () => { await act(() => resolveText('orld')); expect(document.getElementById('app-div').outerHTML).toEqual( - '
hworld
', + '
hworld
', ); await act(() => resolveText('ello')); expect(container.firstElementChild.outerHTML).toEqual( - '
helloworld
', + '
helloworld
', + ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
{['h', 'ello', 'w', 'orld']}
, ); }); @@ -3685,6 +3740,7 @@ describe('ReactDOMFizzServer', () => { await act(async () => { const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await afterImmediate(); await act(() => resolveText('ello')); pipe(writable); }); @@ -3696,7 +3752,19 @@ describe('ReactDOMFizzServer', () => { await act(() => resolveText('orld')); expect(container.firstElementChild.outerHTML).toEqual( - '
helloworld
', + '
helloworld
', + ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
{['h', 'ello', 'w', 'orld']}
, ); }); @@ -3716,12 +3784,27 @@ describe('ReactDOMFizzServer', () => { await act(async () => { const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await afterImmediate(); await act(() => resolveText('world')); pipe(writable); }); expect(container.firstElementChild.outerHTML).toEqual( - '
helloworld
', + '
helloworld
', + ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ helloworld +
, ); }); @@ -3739,6 +3822,7 @@ describe('ReactDOMFizzServer', () => { await act(async () => { const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await afterImmediate(); await act(() => resolveText('world')); pipe(writable); }); @@ -3746,6 +3830,20 @@ describe('ReactDOMFizzServer', () => { expect(container.firstElementChild.outerHTML).toEqual( '
helloworld
', ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ helloworld +
, + ); }); it('should not insert separators for text inside Suspense boundaries even if they would otherwise be considered text-embedded', async () => { @@ -3773,6 +3871,7 @@ describe('ReactDOMFizzServer', () => { await act(async () => { const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await afterImmediate(); await act(() => resolveText('world')); pipe(writable); }); @@ -3786,7 +3885,26 @@ describe('ReactDOMFizzServer', () => { }); expect(document.getElementById('app-div').outerHTML).toEqual( - '
startfirststartfirst suspendedfirstend[loading second]end
', + '
startfirststartfirst suspendedfirstend[loading second]end
', + ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ {'start'} + {'firststart'} + {'first suspended'} + {'firstend'} + {'[loading second]'} + {'end'} +
, ); await act(async () => { @@ -3794,7 +3912,82 @@ describe('ReactDOMFizzServer', () => { }); expect(container.firstElementChild.outerHTML).toEqual( - '
startfirststartfirst suspendedfirstendsecondstartsecond suspendedend
', + '
startfirststartfirst suspendedfirstendsecondstartsecond suspendedend
', + ); + + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ {'start'} + {'firststart'} + {'first suspended'} + {'firstend'} + {'secondstart'} + second suspended + {'end'} +
, + ); + }); + + it('(only) includes extraneous text separators in segments that complete before flushing, followed by nothing or a non-Text node', async () => { + function App() { + return ( +
+ + hello + + + + + + + hello + +
+
+ + +
+
+
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await afterImmediate(); + await act(() => resolveText('world')); + pipe(writable); + }); + + expect(container.innerHTML).toEqual( + '
helloworldworldhelloworld
world
', + ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ {/* first boundary */} + {'hello'} + {'world'} + {/* second boundary */} + {'world'} + {/* third boundary */} + {'hello'} + {'world'} +
+ {/* fourth boundary */} + {'world'} +
+
, ); }); }); diff --git a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js index 44f083a41145..71c490b76aaa 100644 --- a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js @@ -307,17 +307,16 @@ export function pushTextInstance( // advanced with the tracking and drop the trailing edge in some cases when we know a segment // is followed by a Text or non-Text Node explicitly vs followed by another Segment export opaque type TextEmbedding = number; -type Embeddable = {textEmbedding: TextEmbedding, ...}; -const NO_TEXT_EMBED = /* */ 0b0000; +const NO_TEXT_EMBED = /* */ 0b0000; -const LEADING_SEPARATOR_NEEDED = /* */ 0b0011; -const LEADING_TEXT_EMBED_KNOWN = /* */ 0b0001; +const LEADING_SEPARATOR_NEEDED = /* */ 0b0011; +const LEADING_TEXT_EMBED_KNOWN = /* */ 0b0001; // const LEADING_TEXT_EMBED_POSSIBLE = /* */ 0b0010; -const TRAILING_SEPARATOR_NEEDED = /* */ 0b1100; +const TRAILING_SEPARATOR_NEEDED = /* */ 0b1100; // const TRAILING_TEXT_EMBED_KNOWN = /* */ 0b0100; -const TRAILING_TEXT_EMBED_POSSIBLE = /* */ 0b1000; +const TRAILING_TEXT_EMBED_POSSIBLE = /* */ 0b1000; // Suspense boundaries always emit a comment node at the leading and trailing edge and thus need // no additional separators. we also use this for the root segment even though it isn't a Boundary @@ -327,6 +326,8 @@ export function textEmbeddingForBoundarySegment(): TextEmbedding { return NO_TEXT_EMBED; } +// Segments that are not the boundary segment can be truly embedded in Text. we determine the leading edge +// based on what was last pushed. We cannot know the trailing edge so we conservatively assume we are embedded there export function textEmbeddingForSegment(): TextEmbedding { let embedding = TRAILING_TEXT_EMBED_POSSIBLE; embedding |= lastPushedText ? LEADING_TEXT_EMBED_KNOWN : NO_TEXT_EMBED; @@ -335,6 +336,12 @@ export function textEmbeddingForSegment(): TextEmbedding { return embedding; } +// If a Segment is going to be flushed later than it's parent text separators arent needed +// because the DOM patch will leavee the adjacent text as separate nodes +export function textEmbeddingForDelayedSegment(): TextEmbedding { + return NO_TEXT_EMBED; +} + // Called when a segment is about to be rendered by a Task export function prepareForSegment(textEmbedding: TextEmbedding) { lastPushedText = textEmbedding & LEADING_SEPARATOR_NEEDED; diff --git a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js index 8be02f8ea625..9d91215abed1 100644 --- a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js @@ -75,6 +75,7 @@ export function createRootFormatContext(): FormatContext { export type { FormatContext, SuspenseBoundaryID, + TextEmbedding, } from './ReactDOMServerFormatConfig'; export { @@ -97,6 +98,7 @@ export { writeCompletedRoot, textEmbeddingForBoundarySegment, textEmbeddingForSegment, + textEmbeddingForDelayedSegment, prepareForSegment, finalizeForSegment, } from './ReactDOMServerFormatConfig'; diff --git a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js index 79bf325e6a63..a347c7c674c6 100644 --- a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js +++ b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js @@ -155,13 +155,10 @@ export function pushEndInstance( ): void { target.push(END); } - -export function textEmbeddingForBoundarySegment() { - return null; -} -export function textEmbeddingForSegment() { - return null; -} +export opaque type TextEmbedding = void; +export function textEmbeddingForBoundarySegment() {} +export function textEmbeddingForSegment() {} +export function textEmbeddingForDelayedSegment() {} export function prepareForSegment() {} export function finalizeForSegment() {} diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 1e491ad3da88..cd6a67751534 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -62,6 +62,7 @@ import { getChildFormatContext, textEmbeddingForBoundarySegment, textEmbeddingForSegment, + textEmbeddingForDelayedSegment, prepareForSegment, finalizeForSegment, } from './ReactServerFormatConfig'; @@ -175,7 +176,7 @@ type Segment = { // If this segment represents a fallback, this is the content that will replace that fallback. +boundary: null | SuspenseBoundary, // opaque data that tells the formatter uses to modify output based on how the Segment is embedded in other output - +textEmbedding: TextEmbedding, + textEmbedding: TextEmbedding, }; const OPEN = 0; @@ -1652,6 +1653,7 @@ function flushSubtree( // We're emitting a placeholder for this segment to be filled in later. // Therefore we'll need to assign it an ID - to refer to it by. const segmentID = (segment.id = request.nextSegmentId++); + segment.textEmbedding = textEmbeddingForDelayedSegment(); return writePlaceholder(destination, request.responseState, segmentID); } case COMPLETED: { diff --git a/packages/react-server/src/forks/ReactServerFormatConfig.custom.js b/packages/react-server/src/forks/ReactServerFormatConfig.custom.js index 75be9ea57845..bd7987c32297 100644 --- a/packages/react-server/src/forks/ReactServerFormatConfig.custom.js +++ b/packages/react-server/src/forks/ReactServerFormatConfig.custom.js @@ -45,6 +45,8 @@ export const pushEndCompletedSuspenseBoundary = $$$hostConfig.pushEndCompletedSuspenseBoundary; export const textEmbeddingForBoundarySegment = $$$hostConfig.textEmbeddingForBoundarySegment; +export const textEmbeddingForDelayedSegment = + $$$hostConfig.textEmbeddingForDelayedSegment; export const textEmbeddingForSegment = $$$hostConfig.textEmbeddingForSegment; export const prepareForSegment = $$$hostConfig.prepareForSegment; export const finalizeForSegment = $$$hostConfig.finalizeForSegment; From d0660feed6846929b931a868adc005dcf0e13633 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 14:29:25 -0700 Subject: [PATCH 04/10] Remove renderToString hack for supressing textSeparators --- .../src/server/ReactDOMLegacyServerStreamConfig.js | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/packages/react-dom/src/server/ReactDOMLegacyServerStreamConfig.js b/packages/react-dom/src/server/ReactDOMLegacyServerStreamConfig.js index 55418357f447..380ef3de2354 100644 --- a/packages/react-dom/src/server/ReactDOMLegacyServerStreamConfig.js +++ b/packages/react-dom/src/server/ReactDOMLegacyServerStreamConfig.js @@ -36,16 +36,6 @@ export function writeChunkAndReturn( destination: Destination, chunk: Chunk | PrecomputedChunk, ): boolean { - if (prevWasCommentSegmenter) { - prevWasCommentSegmenter = false; - if (chunk[0] !== '<') { - destination.push(''); - } - } - if (chunk === '') { - prevWasCommentSegmenter = true; - return true; - } return destination.push(chunk); } From 3030aecd780a08a7828d8d0b35965956f671463b Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 14:32:01 -0700 Subject: [PATCH 05/10] fixup! Remove renderToString hack for supressing textSeparators --- .../react-dom/src/server/ReactDOMLegacyServerStreamConfig.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/react-dom/src/server/ReactDOMLegacyServerStreamConfig.js b/packages/react-dom/src/server/ReactDOMLegacyServerStreamConfig.js index 380ef3de2354..6f471a5552a5 100644 --- a/packages/react-dom/src/server/ReactDOMLegacyServerStreamConfig.js +++ b/packages/react-dom/src/server/ReactDOMLegacyServerStreamConfig.js @@ -24,7 +24,6 @@ export function flushBuffered(destination: Destination) {} export function beginWriting(destination: Destination) {} -let prevWasCommentSegmenter = false; export function writeChunk( destination: Destination, chunk: Chunk | PrecomputedChunk, From 3f61057d0bde1884d23cf4bc1f14eeae872f576a Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 15:54:59 -0700 Subject: [PATCH 06/10] refactor to move embed status to the segment itself --- .../src/server/ReactDOMServerFormatConfig.js | 76 ++++--------------- .../ReactDOMServerLegacyFormatConfig.js | 12 ++- .../server/ReactNativeServerFormatConfig.js | 8 +- .../src/ReactNoopServer.js | 9 +-- packages/react-server/src/ReactFizzServer.js | 70 ++++++++++++----- .../forks/ReactServerFormatConfig.custom.js | 8 +- 6 files changed, 73 insertions(+), 110 deletions(-) diff --git a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js index 71c490b76aaa..73829d121bef 100644 --- a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js @@ -279,83 +279,35 @@ function encodeHTMLTextNode(text: string): string { } const textSeparator = stringToPrecomputedChunk(''); -let lastPushedText = false; export function pushTextInstance( target: Array, text: string, responseState: ResponseState, -): void { + textEmbedded?: boolean, +): boolean { if (text === '') { // Empty text doesn't have a DOM node representation and the hydration is aware of this. - return; + return textEmbedded; } - // We use a null charater to wrap every text chunk. we use this to boundary check whether we need - // to insert a Node textSeparator like when flushing segments between sets of chunks. - if (lastPushedText) { + if (textEmbedded) { target.push(textSeparator); } - lastPushedText = true; target.push(stringToChunk(encodeHTMLTextNode(text))); + return true; } -// The Text Embedding flags allow the format config to know when to include or exclude -// text separators between parent and child segments. We track the leading edge and -// trailing edges separately however due to the fact that peeking ahead is usually -// not possible we discern between knowing we need a separator at the leading edge and -// assuming we might need one at the trailing edge. In the future we could get more -// advanced with the tracking and drop the trailing edge in some cases when we know a segment -// is followed by a Text or non-Text Node explicitly vs followed by another Segment -export opaque type TextEmbedding = number; - -const NO_TEXT_EMBED = /* */ 0b0000; - -const LEADING_SEPARATOR_NEEDED = /* */ 0b0011; -const LEADING_TEXT_EMBED_KNOWN = /* */ 0b0001; -// const LEADING_TEXT_EMBED_POSSIBLE = /* */ 0b0010; - -const TRAILING_SEPARATOR_NEEDED = /* */ 0b1100; -// const TRAILING_TEXT_EMBED_KNOWN = /* */ 0b0100; -const TRAILING_TEXT_EMBED_POSSIBLE = /* */ 0b1000; - -// Suspense boundaries always emit a comment node at the leading and trailing edge and thus need -// no additional separators. we also use this for the root segment even though it isn't a Boundary -// because it cannot have pre or post text and thus matches the same embedding semantics or Boundaries -export function textEmbeddingForBoundarySegment(): TextEmbedding { - lastPushedText = false; - return NO_TEXT_EMBED; -} - -// Segments that are not the boundary segment can be truly embedded in Text. we determine the leading edge -// based on what was last pushed. We cannot know the trailing edge so we conservatively assume we are embedded there -export function textEmbeddingForSegment(): TextEmbedding { - let embedding = TRAILING_TEXT_EMBED_POSSIBLE; - embedding |= lastPushedText ? LEADING_TEXT_EMBED_KNOWN : NO_TEXT_EMBED; - - lastPushedText = false; - return embedding; -} - -// If a Segment is going to be flushed later than it's parent text separators arent needed -// because the DOM patch will leavee the adjacent text as separate nodes -export function textEmbeddingForDelayedSegment(): TextEmbedding { - return NO_TEXT_EMBED; -} - -// Called when a segment is about to be rendered by a Task -export function prepareForSegment(textEmbedding: TextEmbedding) { - lastPushedText = textEmbedding & LEADING_SEPARATOR_NEEDED; -} - -// Called when a segment is exhaustively rendered -export function finalizeForSegment( +// Called when Fizz is done with a Segment. Currently the only purpose is to conditionally +// emit a text separator when we don't know for sure it is safe to omit +export function pushSegmentFinale( target: Array, - textEmbedding: TextEmbedding, -) { - if (lastPushedText && textEmbedding & TRAILING_SEPARATOR_NEEDED) { + responseState: ResponseState, + lastPushedText: Boolean, + textEmbedded: Boolean, +): void { + if (lastPushedText && textEmbedded) { target.push(textSeparator); } - lastPushedText = false; } const styleNameCache: Map = new Map(); @@ -1389,7 +1341,6 @@ export function pushStartInstance( responseState: ResponseState, formatContext: FormatContext, ): ReactNodeList { - lastPushedText = false; if (__DEV__) { validateARIAProperties(type, props); validateInputProperties(type, props); @@ -1502,7 +1453,6 @@ export function pushEndInstance( type: string, props: Object, ): void { - lastPushedText = false; switch (type) { // Omitted close tags // TODO: Instead of repeating this switch we could try to pass a flag from above. diff --git a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js index 9d91215abed1..207d53e27b80 100644 --- a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js @@ -87,6 +87,7 @@ export { pushEndInstance, pushStartCompletedSuspenseBoundary, pushEndCompletedSuspenseBoundary, + pushSegmentFinale, writeStartSegment, writeEndSegment, writeCompletedSegmentInstruction, @@ -96,11 +97,6 @@ export { writeEndPendingSuspenseBoundary, writePlaceholder, writeCompletedRoot, - textEmbeddingForBoundarySegment, - textEmbeddingForSegment, - textEmbeddingForDelayedSegment, - prepareForSegment, - finalizeForSegment, } from './ReactDOMServerFormatConfig'; import {stringToChunk} from 'react-server/src/ReactServerStreamConfig'; @@ -111,11 +107,13 @@ export function pushTextInstance( target: Array, text: string, responseState: ResponseState, -): void { + textEmbedded: boolean, +): boolean { if (responseState.generateStaticMarkup) { target.push(stringToChunk(escapeTextForBrowser(text))); + return false; } else { - pushTextInstanceImpl(target, text, responseState); + return pushTextInstanceImpl(target, text, responseState, textEmbedded); } } diff --git a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js index a347c7c674c6..a8030a0727d4 100644 --- a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js +++ b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js @@ -155,12 +155,8 @@ export function pushEndInstance( ): void { target.push(END); } -export opaque type TextEmbedding = void; -export function textEmbeddingForBoundarySegment() {} -export function textEmbeddingForSegment() {} -export function textEmbeddingForDelayedSegment() {} -export function prepareForSegment() {} -export function finalizeForSegment() {} + +export function pushSegmentFinale() {} export function writeCompletedRoot( destination: Destination, diff --git a/packages/react-noop-renderer/src/ReactNoopServer.js b/packages/react-noop-renderer/src/ReactNoopServer.js index 4aa3a186d918..f88ab3f0ee6e 100644 --- a/packages/react-noop-renderer/src/ReactNoopServer.js +++ b/packages/react-noop-renderer/src/ReactNoopServer.js @@ -128,14 +128,7 @@ const ReactNoopServer = ReactFizzServer({ target.push(POP); }, - textEmbeddingForBoundarySegment() { - return null; - }, - textEmbeddingForSegment() { - return null; - }, - prepareForSegment() {}, - finalizeForSegment() {}, + pushSegmentFinale() {}, writeCompletedRoot( destination: Destination, diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index cd6a67751534..7908948b49f1 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -26,7 +26,6 @@ import type { import type {ContextSnapshot} from './ReactFizzNewContext'; import type {ComponentStackNode} from './ReactFizzComponentStack'; import type {TreeContext} from './ReactFizzTreeContext'; -import type {TextEmbedding} from './ReactServerFormatConfig'; import { scheduleWork, @@ -57,6 +56,7 @@ import { pushEndInstance, pushStartCompletedSuspenseBoundary, pushEndCompletedSuspenseBoundary, + pushSegmentFinale, UNINITIALIZED_SUSPENSE_BOUNDARY_ID, assignSuspenseBoundaryID, getChildFormatContext, @@ -175,8 +175,9 @@ type Segment = { formatContext: FormatContext, // If this segment represents a fallback, this is the content that will replace that fallback. +boundary: null | SuspenseBoundary, - // opaque data that tells the formatter uses to modify output based on how the Segment is embedded in other output - textEmbedding: TextEmbedding, + // used to discern when text separator boundaries are needed + lastPushedText: boolean, + textEmbedded: boolean, }; const OPEN = 0; @@ -280,7 +281,9 @@ export function createRequest( 0, null, rootFormatContext, - textEmbeddingForBoundarySegment(), + // Root segments are never embedded in Text on either edge + false, + false, ); // There is no parent so conceptually, we're unblocked to flush this segment. rootSegment.parentFlushed = true; @@ -360,7 +363,8 @@ function createPendingSegment( index: number, boundary: null | SuspenseBoundary, formatContext: FormatContext, - textEmbedding: TextEmbedding, + lastPushedText: boolean, + textEmbedded: boolean, ): Segment { return { status: PENDING, @@ -371,7 +375,8 @@ function createPendingSegment( children: [], formatContext, boundary, - textEmbedding, + lastPushedText, + textEmbedded, }; } @@ -470,15 +475,18 @@ function renderSuspenseBoundary( const newBoundary = createSuspenseBoundary(request, fallbackAbortSet); const insertionIndex = parentSegment.chunks.length; // The children of the boundary segment is actually the fallback. - const textEmbedding = textEmbeddingForBoundarySegment(); const boundarySegment = createPendingSegment( request, insertionIndex, newBoundary, parentSegment.formatContext, - textEmbedding, + // boundaries never require text embedding at their edges because comment nodes bound them + false, + false, ); parentSegment.children.push(boundarySegment); + // The parentSegment has a child Segment at this index so we reset the lastPushedText marker on the parent + parentSegment.lastPushedText = false; // This segment is the actual child content. We can start rendering that immediately. const contentRootSegment = createPendingSegment( @@ -486,7 +494,9 @@ function renderSuspenseBoundary( 0, null, parentSegment.formatContext, - textEmbedding, + // boundaries never require text embedding at their edges because comment nodes bound them + false, + false, ); // We mark the root segment as having its parent flushed. It's not really flushed but there is // no parent segment so there's nothing to wait on. @@ -504,11 +514,12 @@ function renderSuspenseBoundary( task.blockedSegment = contentRootSegment; try { // We use the safe form because we don't handle suspending here. Only error handling. - prepareForSegment(contentRootSegment.textEmbedding); renderNode(request, task, content); - finalizeForSegment( + pushSegmentFinale( contentRootSegment.chunks, - contentRootSegment.textEmbedding, + request.responseState, + contentRootSegment.lastPushedText, + contentRootSegment.textEmbedded, ); contentRootSegment.status = COMPLETED; queueCompletedSegment(newBoundary, contentRootSegment); @@ -585,15 +596,18 @@ function renderHostElement( request.responseState, segment.formatContext, ); + segment.lastPushedText = false; const prevContext = segment.formatContext; segment.formatContext = getChildFormatContext(prevContext, type, props); // We use the non-destructive form because if something suspends, we still // need to pop back up and finish this subtree of HTML. renderNode(request, task, children); + // We expect that errors will fatal the whole task and that we don't need // the correct context. Therefore this is not in a finally. segment.formatContext = prevContext; pushEndInstance(segment.chunks, type, props); + segment.lastPushedText = false; popComponentStackInDEV(task); } @@ -1240,15 +1254,23 @@ function renderNodeDestructive( } if (typeof node === 'string') { - pushTextInstance(task.blockedSegment.chunks, node, request.responseState); + let segment = task.blockedSegment; + segment.lastPushedText = pushTextInstance( + task.blockedSegment.chunks, + node, + request.responseState, + segment.lastPushedText, + ); return; } if (typeof node === 'number') { - pushTextInstance( + let segment = task.blockedSegment; + segment.lastPushedText = pushTextInstance( task.blockedSegment.chunks, '' + node, request.responseState, + segment.lastPushedText, ); return; } @@ -1287,15 +1309,19 @@ function spawnNewSuspendedTask( // Something suspended, we'll need to create a new segment and resolve it later. const segment = task.blockedSegment; const insertionIndex = segment.chunks.length; - const textEmbedding = textEmbeddingForSegment(); const newSegment = createPendingSegment( request, insertionIndex, null, segment.formatContext, - textEmbedding, + // Adopt the parent segment's leading text embed + segment.lastPushedText, + // Assume we are text embedded at the trailing edge + true, ); segment.children.push(newSegment); + // Reset lastPushedText for current Segment since the new Segment "consumed" it + segment.lastPushedText = false; const newTask = createTask( request, task.node, @@ -1570,9 +1596,13 @@ function retryTask(request: Request, task: Task): void { try { // We call the destructive form that mutates this task. That way if something // suspends again, we can reuse the same task instead of spawning a new one. - prepareForSegment(segment.textEmbedding); renderNodeDestructive(request, task, task.node); - finalizeForSegment(segment.chunks, segment.textEmbedding); + pushSegmentFinale( + segment.chunks, + request.responseState, + segment.lastPushedText, + segment.textEmbedded, + ); task.abortSet.delete(task); segment.status = COMPLETED; @@ -1653,7 +1683,9 @@ function flushSubtree( // We're emitting a placeholder for this segment to be filled in later. // Therefore we'll need to assign it an ID - to refer to it by. const segmentID = (segment.id = request.nextSegmentId++); - segment.textEmbedding = textEmbeddingForDelayedSegment(); + // When this segment finally completes it won't be embedded in text since it will flush separately + segment.lastPushedText = false; + segment.textEmbedded = false; return writePlaceholder(destination, request.responseState, segmentID); } case COMPLETED: { diff --git a/packages/react-server/src/forks/ReactServerFormatConfig.custom.js b/packages/react-server/src/forks/ReactServerFormatConfig.custom.js index bd7987c32297..ecb3218ea1da 100644 --- a/packages/react-server/src/forks/ReactServerFormatConfig.custom.js +++ b/packages/react-server/src/forks/ReactServerFormatConfig.custom.js @@ -43,13 +43,7 @@ export const pushStartCompletedSuspenseBoundary = $$$hostConfig.pushStartCompletedSuspenseBoundary; export const pushEndCompletedSuspenseBoundary = $$$hostConfig.pushEndCompletedSuspenseBoundary; -export const textEmbeddingForBoundarySegment = - $$$hostConfig.textEmbeddingForBoundarySegment; -export const textEmbeddingForDelayedSegment = - $$$hostConfig.textEmbeddingForDelayedSegment; -export const textEmbeddingForSegment = $$$hostConfig.textEmbeddingForSegment; -export const prepareForSegment = $$$hostConfig.prepareForSegment; -export const finalizeForSegment = $$$hostConfig.finalizeForSegment; +export const pushSegmentFinale = $$$hostConfig.pushSegmentFinale; export const writeCompletedRoot = $$$hostConfig.writeCompletedRoot; export const writePlaceholder = $$$hostConfig.writePlaceholder; export const writeStartCompletedSuspenseBoundary = From f7c4b867a44d27b59ca429688a354d20063306b8 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 16:02:20 -0700 Subject: [PATCH 07/10] flow and lint --- .../react-dom/src/__tests__/ReactDOMFizzServer-test.js | 8 ++++++++ .../react-dom/src/server/ReactDOMServerFormatConfig.js | 6 +++--- .../src/server/ReactDOMServerLegacyFormatConfig.js | 1 - packages/react-server/src/ReactFizzServer.js | 9 ++------- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index fd74ff41518d..56ae7c8a0965 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -3592,6 +3592,7 @@ describe('ReactDOMFizzServer', () => { }); } + // @gate experimental it('it only includes separators between adjacent text nodes', async () => { function App({name}) { return ( @@ -3626,6 +3627,7 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate experimental it('it does not insert text separators even when adjacent text is in a delayed segment', async () => { function App({name}) { return ( @@ -3681,6 +3683,7 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate experimental it('it works with multiple adjacent segments', async () => { function App() { return ( @@ -3726,6 +3729,7 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate experimental it('it works when some segments are flushed and others are patched', async () => { function App() { return ( @@ -3768,6 +3772,7 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate experimental it('it does not prepend a text separators if the segment follows a non-Text Node', async () => { function App() { return ( @@ -3808,6 +3813,7 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate experimental it('it does not prepend a text separators if the segments first emission is a non-Text Node', async () => { function App() { return ( @@ -3846,6 +3852,7 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate experimental it('should not insert separators for text inside Suspense boundaries even if they would otherwise be considered text-embedded', async () => { function App() { return ( @@ -3930,6 +3937,7 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate experimental it('(only) includes extraneous text separators in segments that complete before flushing, followed by nothing or a non-Text node', async () => { function App() { return ( diff --git a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js index 73829d121bef..06fda6a65709 100644 --- a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js @@ -284,7 +284,7 @@ export function pushTextInstance( target: Array, text: string, responseState: ResponseState, - textEmbedded?: boolean, + textEmbedded: boolean, ): boolean { if (text === '') { // Empty text doesn't have a DOM node representation and the hydration is aware of this. @@ -302,8 +302,8 @@ export function pushTextInstance( export function pushSegmentFinale( target: Array, responseState: ResponseState, - lastPushedText: Boolean, - textEmbedded: Boolean, + lastPushedText: boolean, + textEmbedded: boolean, ): void { if (lastPushedText && textEmbedded) { target.push(textSeparator); diff --git a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js index 207d53e27b80..dfee22ed3d46 100644 --- a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js @@ -75,7 +75,6 @@ export function createRootFormatContext(): FormatContext { export type { FormatContext, SuspenseBoundaryID, - TextEmbedding, } from './ReactDOMServerFormatConfig'; export { diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 7908948b49f1..5b0a3de560a6 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -60,11 +60,6 @@ import { UNINITIALIZED_SUSPENSE_BOUNDARY_ID, assignSuspenseBoundaryID, getChildFormatContext, - textEmbeddingForBoundarySegment, - textEmbeddingForSegment, - textEmbeddingForDelayedSegment, - prepareForSegment, - finalizeForSegment, } from './ReactServerFormatConfig'; import { constructClassInstance, @@ -1254,7 +1249,7 @@ function renderNodeDestructive( } if (typeof node === 'string') { - let segment = task.blockedSegment; + const segment = task.blockedSegment; segment.lastPushedText = pushTextInstance( task.blockedSegment.chunks, node, @@ -1265,7 +1260,7 @@ function renderNodeDestructive( } if (typeof node === 'number') { - let segment = task.blockedSegment; + const segment = task.blockedSegment; segment.lastPushedText = pushTextInstance( task.blockedSegment.chunks, '' + node, From 19c76fa0ebc180d1829e58b7ce6f46ab5cca347d Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 16:21:59 -0700 Subject: [PATCH 08/10] cleanup flow some more --- .../ReactDOMServerLegacyFormatConfig.js | 20 ++++++++++++++++++- .../server/ReactNativeServerFormatConfig.js | 8 ++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js index dfee22ed3d46..9d430f7b482c 100644 --- a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js @@ -12,6 +12,7 @@ import type {FormatContext} from './ReactDOMServerFormatConfig'; import { createResponseState as createResponseStateImpl, pushTextInstance as pushTextInstanceImpl, + pushSegmentFinale as pushSegmentFinaleImpl, writeStartCompletedSuspenseBoundary as writeStartCompletedSuspenseBoundaryImpl, writeStartClientRenderedSuspenseBoundary as writeStartClientRenderedSuspenseBoundaryImpl, writeEndCompletedSuspenseBoundary as writeEndCompletedSuspenseBoundaryImpl, @@ -86,7 +87,6 @@ export { pushEndInstance, pushStartCompletedSuspenseBoundary, pushEndCompletedSuspenseBoundary, - pushSegmentFinale, writeStartSegment, writeEndSegment, writeCompletedSegmentInstruction, @@ -116,6 +116,24 @@ export function pushTextInstance( } } +export function pushSegmentFinale( + target: Array, + responseState: ResponseState, + lastPushedText: boolean, + textEmbedded: boolean, +): void { + if (responseState.generateStaticMarkup) { + return; + } else { + return pushSegmentFinaleImpl( + target, + responseState, + lastPushedText, + textEmbedded, + ); + } +} + export function writeStartCompletedSuspenseBoundary( destination: Destination, responseState: ResponseState, diff --git a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js index a8030a0727d4..73fe677c837d 100644 --- a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js +++ b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js @@ -122,7 +122,9 @@ export function pushTextInstance( target: Array, text: string, responseState: ResponseState, -): void { + // This Renderer does not use this argument + textEmbedded: mixed, +): boolean { target.push( INSTANCE, RAW_TEXT, // Type @@ -130,6 +132,7 @@ export function pushTextInstance( // TODO: props { text: text } END, // End of children ); + return false; } export function pushStartInstance( @@ -156,7 +159,8 @@ export function pushEndInstance( target.push(END); } -export function pushSegmentFinale() {} +// In this Renderer this is a noop +export function pushSegmentFinale(a: mixed, b: mixed, c: mixed, d: mixed) {} export function writeCompletedRoot( destination: Destination, From fc76c7ff1072f66ceb43921c49e3328b9be589e4 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 16:27:52 -0700 Subject: [PATCH 09/10] more flow refinement --- .../src/server/ReactNativeServerFormatConfig.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js index 73fe677c837d..470fed5ce6d0 100644 --- a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js +++ b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js @@ -123,7 +123,7 @@ export function pushTextInstance( text: string, responseState: ResponseState, // This Renderer does not use this argument - textEmbedded: mixed, + textEmbedded: boolean, ): boolean { target.push( INSTANCE, @@ -160,7 +160,12 @@ export function pushEndInstance( } // In this Renderer this is a noop -export function pushSegmentFinale(a: mixed, b: mixed, c: mixed, d: mixed) {} +export function pushSegmentFinale( + target: Array, + responseState: ResponseState, + lastPushedText: boolean, + textEmbedded: boolean, +): void {} export function writeCompletedRoot( destination: Destination, From 277beb838e6de79e8da00e85593b9005be897439 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 27 May 2022 16:36:35 -0700 Subject: [PATCH 10/10] cleanup ReactNoopServer --- .../react-noop-renderer/src/ReactNoopServer.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/react-noop-renderer/src/ReactNoopServer.js b/packages/react-noop-renderer/src/ReactNoopServer.js index f88ab3f0ee6e..8cbd0c84dba7 100644 --- a/packages/react-noop-renderer/src/ReactNoopServer.js +++ b/packages/react-noop-renderer/src/ReactNoopServer.js @@ -98,12 +98,18 @@ const ReactNoopServer = ReactFizzServer({ return null; }, - pushTextInstance(target: Array, text: string): void { + pushTextInstance( + target: Array, + text: string, + responseState: ResponseState, + textEmbedded: boolean, + ): boolean { const textInstance: TextInstance = { text, hidden: false, }; target.push(Buffer.from(JSON.stringify(textInstance), 'utf8'), POP); + return false; }, pushStartInstance( target: Array, @@ -128,7 +134,13 @@ const ReactNoopServer = ReactFizzServer({ target.push(POP); }, - pushSegmentFinale() {}, + // This is a noop in ReactNoop + pushSegmentFinale( + target: Array, + responseState: ResponseState, + lastPushedText: boolean, + textEmbedded: boolean, + ): void {}, writeCompletedRoot( destination: Destination,