From e85a292da8fb697bf06b4570e239ed0b2a093fd8 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sun, 29 May 2022 14:05:52 -0700 Subject: [PATCH 1/4] [Fizz] deterministic text separators This change addresses two related flaws in the current approach to text separators in Fizz. First text separators can be emitted more often than necessary. This is because Segments don't know whether they will be followed by text and so if they end in text they assume they are and insert a separators Second, becuase of the flaw above, Fizz output is not deterministic. If you wait for everything to be ready before flushing you could still end up with different output depending on whether certain segments are created or rendered inline based on the particular behavior of Suspendable components for any given run. If a Segment emits an unecessary text separator in one pass and in another the Segment is never created because that Component did not Suspend the string output would differ for otherwise identical content This change fixes both issues by decorating Segments with additional metadata about what kinds of emitted content is at it's bounds. These are called Edges and can be NODE, TEXT, or a Segment. NODE and TEXT are type identifiers that refer generically to these mutually exclusive concepts. if an edge is a Segment however it is the actual reference to the Segment object, which allows us to make inferrences at flushing time as to whether additional text separators are needed. There are 2 tracked edges per Segment currentEdge - a pointer to type of the prior thing emitted for a Segment. If the segment has not yet been worked on this will be the Edge just before the Segment start. If the Segment is currently being rendered it will be the type or reference of the last thing this Segment emitted. If the Segment is complete it definitionally be the trailing edge of the Segment representing the type emitted at the boundary. followingEdge - this edge is set by later Segments when the currentEdge of that segment is a Segment. If the currentEdge is a Segment and a new Edge is identified it is saved to the followingEdge of the currentEdge segment. Finally, when flushing 2 things happen If the just written Segment ends with text (currentEdge is TEXT_NODE) and if the followingEdge is also text, then a separator is written in between. If the followingEdge is instead another Completed Segment this segment is empty and we walk to that Segment's followingEdge and continue looking for Text. --- .../src/__tests__/ReactDOMFizzServer-test.js | 361 +++++++++++++++++- .../ReactDOMFizzServerBrowser-test.js | 4 +- .../__tests__/ReactDOMFizzServerNode-test.js | 2 +- .../src/server/ReactDOMServerFormatConfig.js | 23 +- .../ReactDOMServerLegacyFormatConfig.js | 29 +- .../server/ReactNativeServerFormatConfig.js | 14 +- .../src/ReactNoopServer.js | 10 +- packages/react-server/src/ReactFizzServer.js | 142 ++++--- .../forks/ReactServerFormatConfig.custom.js | 2 +- 9 files changed, 477 insertions(+), 110 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 56ae7c8a09652..11d4eaa154f22 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -3795,7 +3795,7 @@ describe('ReactDOMFizzServer', () => { }); expect(container.firstElementChild.outerHTML).toEqual( - '
helloworld
', + '
helloworld
', ); const errors = []; @@ -3938,7 +3938,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 () => { + it('excludes extraneous text separators in segments that complete before flushing, followed by nothing or a non-Text node', async () => { function App() { return (
@@ -3970,7 +3970,7 @@ describe('ReactDOMFizzServer', () => { }); expect(container.innerHTML).toEqual( - '
helloworldworldhelloworld
world
', + '
helloworldworldhelloworld
world
', ); const errors = []; @@ -3998,5 +3998,360 @@ describe('ReactDOMFizzServer', () => {
, ); }); + + // @gate experimental + it('handles many serial adjacent segments that resolves in arbitrary order', async () => { + function NineText() { + return ( + <> + + + + + ); + } + + function ThreeText({start}) { + return ( + <> + + + + + ); + } + + function App() { + return ( +
+ + + +
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await act(() => resolveText(1)); + await act(() => resolveText(6)); + await act(() => resolveText(9)); + await afterImmediate(); + await act(() => resolveText(2)); + await act(() => resolveText(5)); + await act(() => resolveText(7)); + pipe(writable); + }); + + expect(container.innerHTML).toEqual( + '
', + ); + + await act(async () => { + resolveText(3); + resolveText(4); + resolveText(8); + }); + + expect(container.firstElementChild.outerHTML).toEqual( + '
123456789
', + ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ {'1'} + {'2'} + {'3'} + {'4'} + {'5'} + {'6'} + {'7'} + {'8'} + {'9'} +
, + ); + }); + + // @gate experimental + it('handles deeply nested segments that resolves in arbitrary order', async () => { + function RecursiveNumber({from, steps, reverse}) { + if (steps === 1) { + return readText(from); + } + + const num = readText(from); + + return ( + <> + {num} + + + ); + } + + function App() { + return ( +
+ + + + +
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await afterImmediate(); + await act(() => resolveText(1)); + await act(() => resolveText(2)); + await act(() => resolveText(4)); + + pipe(writable); + }); + + expect(container.innerHTML).toEqual( + '
', + ); + + await act(async () => { + resolveText(3); + resolveText(5); + resolveText(6); + }); + + expect(container.firstElementChild.outerHTML).toEqual( + '
123654
', + ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ {'1'} + {'2'} + {'3'} + {'6'} + {'5'} + {'4'} +
, + ); + }); + + // @gate experimental + it('handles segments that return null', async () => { + function WrappedAsyncText({outer, text}) { + readText(outer); + return ; + } + + function App() { + return ( +
+ +
+ + + +
+
+ + + +
+
+ + + +
+
+ + + + +
+
+ + + + +
+
+ + + +
+ +
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await afterImmediate(); + await act(() => resolveText('outer2')); + await act(() => resolveText('world')); + await act(() => resolveText('outer1')); + await act(() => resolveText(null)); + await act(() => resolveText('hello')); + + pipe(writable); + }); + + let helloWorld = '
helloworld
'; + let testcases = 6; + + expect(container.firstElementChild.outerHTML).toEqual( + '
' + + new Array(testcases).fill(helloWorld).join('') + + '
', + ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + const assertion = () => { + expect(getVisibleChildren(container)).toEqual( +
+ {new Array(testcases).fill( +
+ {'hello'} + {'world'} +
, + )} +
, + ); + }; + if (__DEV__) { + expect(assertion).toErrorDev([ + 'Warning: Each child in a list should have a unique "key" prop.', + ]); + } else { + assertion(); + } + }); + + // @gate experimental + it('does not add separators when otherwise adjacent text is wrapped in Suspense', async () => { + function App() { + return ( +
+ hello + + + +
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await afterImmediate(); + await act(() => resolveText('world')); + + pipe(writable); + }); + + 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( +
+ {'hello'} + {'world'} +
, + ); + }); + + // @gate experimental + it('does not prepend separators for Suspense fallback text but will append them if followed by text', async () => { + function App() { + return ( +
+ + hello + + ! + + }> + + + ! + +
+ ); + } + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream(); + await afterImmediate(); + await act(() => resolveText('foo')); + pipe(writable); + }); + + expect(container.innerHTML).toEqual( + '
outer
', + ); + + await act(() => resolveText('world')); + + expect(container.children[0].outerHTML).toEqual( + '
helloworld!foo!
', + ); + + const errors = []; + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error.message); + }, + }); + expect(Scheduler).toFlushAndYield([]); + expect(errors).toEqual([]); + expect(getVisibleChildren(container)).toEqual( +
+ {'hello'} + {/* starting the inner Suspense boundary Fallback */} + {'world'} + {'!'} + {'foo'} + {/* ending the inner Suspense boundary Fallback */} + {'!'} +
, + ); + }); }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js index 792b9611f29f2..11ef1ee1823c6 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js @@ -119,9 +119,7 @@ describe('ReactDOMFizzServer', () => { expect(isComplete).toBe(true); const result = await readResult(stream); - expect(result).toMatchInlineSnapshot( - `"
Done
"`, - ); + expect(result).toMatchInlineSnapshot(`"
Done
"`); }); // @gate experimental diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js index c1844d7ef78bf..9de3b17b0a21a 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js @@ -161,7 +161,7 @@ describe('ReactDOMFizzServer', () => { // Then React starts writing. pipe(writable); expect(output.result).toMatchInlineSnapshot( - `"test
Done
"`, + `"test
Done
"`, ); }); diff --git a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js index 06fda6a65709c..00499f13050c7 100644 --- a/packages/react-dom/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerFormatConfig.js @@ -280,6 +280,7 @@ function encodeHTMLTextNode(text: string): string { const textSeparator = stringToPrecomputedChunk(''); +// Returns a boolean signifying whether text was actually pushed. export function pushTextInstance( target: Array, text: string, @@ -288,7 +289,7 @@ export function pushTextInstance( ): boolean { if (text === '') { // Empty text doesn't have a DOM node representation and the hydration is aware of this. - return textEmbedded; + return false; } if (textEmbedded) { target.push(textSeparator); @@ -297,19 +298,6 @@ export function pushTextInstance( return true; } -// 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, - responseState: ResponseState, - lastPushedText: boolean, - textEmbedded: boolean, -): void { - if (lastPushedText && textEmbedded) { - target.push(textSeparator); - } -} - const styleNameCache: Map = new Map(); function processStyleName(styleName: string): PrecomputedChunk { const chunk = styleNameCache.get(styleName); @@ -1713,6 +1701,13 @@ export function writeEndSegment( } } +export function writeTextSeparator( + destination: Destination, + responseState: ResponseState, +): boolean { + return writeChunkAndReturn(destination, textSeparator); +} + // Instruction Set // The following code is the source scripts that we then minify and inline below, diff --git a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js index 9d430f7b482c1..e64d46eeffb9f 100644 --- a/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js +++ b/packages/react-dom/src/server/ReactDOMServerLegacyFormatConfig.js @@ -12,11 +12,11 @@ import type {FormatContext} from './ReactDOMServerFormatConfig'; import { createResponseState as createResponseStateImpl, pushTextInstance as pushTextInstanceImpl, - pushSegmentFinale as pushSegmentFinaleImpl, writeStartCompletedSuspenseBoundary as writeStartCompletedSuspenseBoundaryImpl, writeStartClientRenderedSuspenseBoundary as writeStartClientRenderedSuspenseBoundaryImpl, writeEndCompletedSuspenseBoundary as writeEndCompletedSuspenseBoundaryImpl, writeEndClientRenderedSuspenseBoundary as writeEndClientRenderedSuspenseBoundaryImpl, + writeTextSeparator as writeTextSeparatorImpl, HTML_MODE, } from './ReactDOMServerFormatConfig'; @@ -116,24 +116,6 @@ 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, @@ -177,3 +159,12 @@ export function writeEndClientRenderedSuspenseBoundary( } return writeEndClientRenderedSuspenseBoundaryImpl(destination, responseState); } +export function writeTextSeparator( + destination: Destination, + responseState: ResponseState, +): boolean { + if (responseState.generateStaticMarkup) { + return true; + } + return writeTextSeparatorImpl(destination, responseState); +} diff --git a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js index 470fed5ce6d0a..265ca1caf0912 100644 --- a/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js +++ b/packages/react-native-renderer/src/server/ReactNativeServerFormatConfig.js @@ -159,14 +159,6 @@ export function pushEndInstance( target.push(END); } -// In this Renderer this is a noop -export function pushSegmentFinale( - target: Array, - responseState: ResponseState, - lastPushedText: boolean, - textEmbedded: boolean, -): void {} - export function writeCompletedRoot( destination: Destination, responseState: ResponseState, @@ -267,6 +259,12 @@ export function writeEndSegment( ): boolean { return writeChunkAndReturn(destination, END); } +export function writeTextSeparator( + destination: Destination, + responseState: ResponseState, +): boolean { + return true; +} // Instruction Set diff --git a/packages/react-noop-renderer/src/ReactNoopServer.js b/packages/react-noop-renderer/src/ReactNoopServer.js index 8cbd0c84dba71..f0afe0e2b7066 100644 --- a/packages/react-noop-renderer/src/ReactNoopServer.js +++ b/packages/react-noop-renderer/src/ReactNoopServer.js @@ -134,14 +134,6 @@ const ReactNoopServer = ReactFizzServer({ target.push(POP); }, - // This is a noop in ReactNoop - pushSegmentFinale( - target: Array, - responseState: ResponseState, - lastPushedText: boolean, - textEmbedded: boolean, - ): void {}, - writeCompletedRoot( destination: Destination, responseState: ResponseState, @@ -220,6 +212,8 @@ const ReactNoopServer = ReactFizzServer({ destination.stack.pop(); }, + writeTextSeparator(destination: Destination, responseState: ResponseState) {}, + writeCompletedSegmentInstruction( destination: Destination, responseState: ResponseState, diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 5b0a3de560a66..da7400fc1675a 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -48,6 +48,7 @@ import { writeEndClientRenderedSuspenseBoundary, writeStartSegment, writeEndSegment, + writeTextSeparator, writeClientRenderBoundaryInstruction, writeCompletedBoundaryInstruction, writeCompletedSegmentInstruction, @@ -56,7 +57,6 @@ import { pushEndInstance, pushStartCompletedSuspenseBoundary, pushEndCompletedSuspenseBoundary, - pushSegmentFinale, UNINITIALIZED_SUSPENSE_BOUNDARY_ID, assignSuspenseBoundaryID, getChildFormatContext, @@ -159,6 +159,14 @@ const ERRORED = 4; type Root = null; +const TEXT_EDGE = 0; +const NODE_EDGE = 1; + +// A SegmentEdge is a descriptor for the kind of object on either side of a segments boundary (leading and trailing). +// In some cases the Edge type isn't known because the edge is a child segment itself. For these cases we use +// the child segment to identify as the edge unless and until a known edge type is found +type SegmentEdge = 0 | 1 | Segment; + type Segment = { status: 0 | 1 | 2 | 3 | 4, parentFlushed: boolean, // typically a segment will be flushed by its parent, except if its parent was already flushed @@ -171,8 +179,8 @@ type Segment = { // If this segment represents a fallback, this is the content that will replace that fallback. +boundary: null | SuspenseBoundary, // used to discern when text separator boundaries are needed - lastPushedText: boolean, - textEmbedded: boolean, + currentEdge: SegmentEdge, + followingEdge: ?SegmentEdge, }; const OPEN = 0; @@ -276,9 +284,8 @@ export function createRequest( 0, null, rootFormatContext, - // Root segments are never embedded in Text on either edge - false, - false, + // Root segments are never embedded in anything but NODE_EDGE has the semantics we need + NODE_EDGE, ); // There is no parent so conceptually, we're unblocked to flush this segment. rootSegment.parentFlushed = true; @@ -358,8 +365,7 @@ function createPendingSegment( index: number, boundary: null | SuspenseBoundary, formatContext: FormatContext, - lastPushedText: boolean, - textEmbedded: boolean, + currentEdge: SegmentEdge, ): Segment { return { status: PENDING, @@ -370,8 +376,8 @@ function createPendingSegment( children: [], formatContext, boundary, - lastPushedText, - textEmbedded, + currentEdge, + followingEdge: null, }; } @@ -459,6 +465,10 @@ function renderSuspenseBoundary( const parentBoundary = task.blockedBoundary; const parentSegment = task.blockedSegment; + // Regardless of whether the boundary segment renders text at it's edges it + // will be wrapped in comment nodes so we can set the currentEdge to NODE_EDGE + parentSegment.currentEdge = NODE_EDGE; + // Each time we enter a suspense boundary, we split out into a new segment for // the fallback so that we can later replace that segment with the content. // This also lets us split out the main content even if it doesn't suspend, @@ -475,13 +485,12 @@ function renderSuspenseBoundary( insertionIndex, newBoundary, parentSegment.formatContext, - // boundaries never require text embedding at their edges because comment nodes bound them - false, - false, + // Boudnaries are wrapped in comment nodes so regardless of the parent segment's currentEdge we + // use a NODE_EDGE here + NODE_EDGE, ); + boundarySegment.followingEdge = NODE_EDGE; 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( @@ -489,10 +498,11 @@ function renderSuspenseBoundary( 0, null, parentSegment.formatContext, - // boundaries never require text embedding at their edges because comment nodes bound them - false, - false, + // Boudnaries are wrapped in comment nodes so regardless of the parent segment's currentEdge we + // use a NODE_EDGE here + NODE_EDGE, ); + contentRootSegment.followingEdge = NODE_EDGE; // 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. contentRootSegment.parentFlushed = true; @@ -510,12 +520,6 @@ function renderSuspenseBoundary( try { // We use the safe form because we don't handle suspending here. Only error handling. renderNode(request, task, content); - pushSegmentFinale( - contentRootSegment.chunks, - request.responseState, - contentRootSegment.lastPushedText, - contentRootSegment.textEmbedded, - ); contentRootSegment.status = COMPLETED; queueCompletedSegment(newBoundary, contentRootSegment); if (newBoundary.pendingTasks === 0) { @@ -591,7 +595,10 @@ function renderHostElement( request.responseState, segment.formatContext, ); - segment.lastPushedText = false; + if (typeof segment.currentEdge === 'object') { + segment.currentEdge.followingEdge = NODE_EDGE; + } + segment.currentEdge = NODE_EDGE; const prevContext = segment.formatContext; segment.formatContext = getChildFormatContext(prevContext, type, props); // We use the non-destructive form because if something suspends, we still @@ -602,7 +609,11 @@ function renderHostElement( // the correct context. Therefore this is not in a finally. segment.formatContext = prevContext; pushEndInstance(segment.chunks, type, props); - segment.lastPushedText = false; + + if (typeof segment.currentEdge === 'object') { + segment.currentEdge.followingEdge = NODE_EDGE; + } + segment.currentEdge = NODE_EDGE; popComponentStackInDEV(task); } @@ -1250,23 +1261,37 @@ function renderNodeDestructive( if (typeof node === 'string') { const segment = task.blockedSegment; - segment.lastPushedText = pushTextInstance( - task.blockedSegment.chunks, - node, - request.responseState, - segment.lastPushedText, - ); + if ( + pushTextInstance( + task.blockedSegment.chunks, + node, + request.responseState, + segment.currentEdge === TEXT_EDGE, + ) + ) { + if (typeof segment.currentEdge === 'object') { + segment.currentEdge.followingEdge = TEXT_EDGE; + } + segment.currentEdge = TEXT_EDGE; + } return; } if (typeof node === 'number') { const segment = task.blockedSegment; - segment.lastPushedText = pushTextInstance( - task.blockedSegment.chunks, - '' + node, - request.responseState, - segment.lastPushedText, - ); + if ( + pushTextInstance( + task.blockedSegment.chunks, + '' + node, + request.responseState, + segment.currentEdge === TEXT_EDGE, + ) + ) { + if (typeof segment.currentEdge === 'object') { + segment.currentEdge.followingEdge = TEXT_EDGE; + } + segment.currentEdge = TEXT_EDGE; + } return; } @@ -1310,13 +1335,14 @@ function spawnNewSuspendedTask( null, segment.formatContext, // Adopt the parent segment's leading text embed - segment.lastPushedText, - // Assume we are text embedded at the trailing edge - true, + segment.currentEdge, ); + // Advance the currentEdge to point to this new Segment. + if (typeof segment.currentEdge === 'object') { + segment.currentEdge.followingEdge = newSegment; + } + segment.currentEdge = newSegment; segment.children.push(newSegment); - // Reset lastPushedText for current Segment since the new Segment "consumed" it - segment.lastPushedText = false; const newTask = createTask( request, task.node, @@ -1592,15 +1618,8 @@ function retryTask(request: Request, task: Task): void { // 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. renderNodeDestructive(request, task, task.node); - pushSegmentFinale( - segment.chunks, - request.responseState, - segment.lastPushedText, - segment.textEmbedded, - ); - - task.abortSet.delete(task); segment.status = COMPLETED; + task.abortSet.delete(task); finishedTask(request, task.blockedBoundary, segment); } catch (x) { resetHooksState(); @@ -1679,8 +1698,10 @@ function flushSubtree( // Therefore we'll need to assign it an ID - to refer to it by. const segmentID = (segment.id = request.nextSegmentId++); // When this segment finally completes it won't be embedded in text since it will flush separately - segment.lastPushedText = false; - segment.textEmbedded = false; + // The currentEdge of a Pending Segment is the Edge just before the Segment begins because we have not + // retried the task for this segment yet. + segment.currentEdge = NODE_EDGE; + segment.followingEdge = NODE_EDGE; return writePlaceholder(destination, request.responseState, segmentID); } case COMPLETED: { @@ -1696,6 +1717,21 @@ function flushSubtree( writeChunk(destination, chunks[chunkIdx]); } r = flushSegment(request, destination, nextChild); + if (nextChild.currentEdge === TEXT_EDGE) { + let followingEdge = nextChild.followingEdge; + // Loop over followingEdges that are Segments until you find somethign that isn't a Completed Segment + // The reason we can halt at pending segments is we know they will emit a placeholder and thus act + // like NODE_EDGE + while (followingEdge !== null && typeof followingEdge === 'object') { + if (followingEdge.status === PENDING) { + break; + } + followingEdge = followingEdge.followingEdge; + } + if (followingEdge === TEXT_EDGE) { + r = writeTextSeparator(destination, request.responseState); + } + } } // Finally just write all the remaining chunks for (; chunkIdx < chunks.length - 1; chunkIdx++) { diff --git a/packages/react-server/src/forks/ReactServerFormatConfig.custom.js b/packages/react-server/src/forks/ReactServerFormatConfig.custom.js index ecb3218ea1dad..dcfa63b13377a 100644 --- a/packages/react-server/src/forks/ReactServerFormatConfig.custom.js +++ b/packages/react-server/src/forks/ReactServerFormatConfig.custom.js @@ -43,7 +43,6 @@ export const pushStartCompletedSuspenseBoundary = $$$hostConfig.pushStartCompletedSuspenseBoundary; export const pushEndCompletedSuspenseBoundary = $$$hostConfig.pushEndCompletedSuspenseBoundary; -export const pushSegmentFinale = $$$hostConfig.pushSegmentFinale; export const writeCompletedRoot = $$$hostConfig.writeCompletedRoot; export const writePlaceholder = $$$hostConfig.writePlaceholder; export const writeStartCompletedSuspenseBoundary = @@ -60,6 +59,7 @@ export const writeEndClientRenderedSuspenseBoundary = $$$hostConfig.writeEndClientRenderedSuspenseBoundary; export const writeStartSegment = $$$hostConfig.writeStartSegment; export const writeEndSegment = $$$hostConfig.writeEndSegment; +export const writeTextSeparator = $$$hostConfig.writeTextSeparator; export const writeCompletedSegmentInstruction = $$$hostConfig.writeCompletedSegmentInstruction; export const writeCompletedBoundaryInstruction = From f021357456fbd168444edc0f4401f54f7d75c3d6 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sun, 29 May 2022 14:39:55 -0700 Subject: [PATCH 2/4] lints --- packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 11d4eaa154f22..a4f9c7c5d6adb 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -4216,8 +4216,8 @@ describe('ReactDOMFizzServer', () => { pipe(writable); }); - let helloWorld = '
helloworld
'; - let testcases = 6; + const helloWorld = '
helloworld
'; + const testcases = 6; expect(container.firstElementChild.outerHTML).toEqual( '
' + From b1420bbcab82506072fccc73edb1e09cd351881d Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 31 May 2022 14:03:49 -0700 Subject: [PATCH 3/4] refactor to remove object linking and property type variance --- packages/react-server/src/ReactFizzServer.js | 155 +++++++++++++------ 1 file changed, 108 insertions(+), 47 deletions(-) diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index da7400fc1675a..aa1e12c28f07b 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -159,13 +159,11 @@ const ERRORED = 4; type Root = null; -const TEXT_EDGE = 0; -const NODE_EDGE = 1; - -// A SegmentEdge is a descriptor for the kind of object on either side of a segments boundary (leading and trailing). -// In some cases the Edge type isn't known because the edge is a child segment itself. For these cases we use -// the child segment to identify as the edge unless and until a known edge type is found -type SegmentEdge = 0 | 1 | Segment; +const UNKNOWN_EDGE = 0; +const SEGMENT_EDGE = 1; +const NODE_EDGE = 2; +const TEXT_EDGE = 3; +type SegmentEdge = 0 | 1 | 2 | 3; type Segment = { status: 0 | 1 | 2 | 3 | 4, @@ -179,8 +177,10 @@ type Segment = { // If this segment represents a fallback, this is the content that will replace that fallback. +boundary: null | SuspenseBoundary, // used to discern when text separator boundaries are needed - currentEdge: SegmentEdge, - followingEdge: ?SegmentEdge, + preEdge: SegmentEdge, + leadEdge: SegmentEdge, + trailEdge: SegmentEdge, + postEdge: SegmentEdge, }; const OPEN = 0; @@ -370,14 +370,17 @@ function createPendingSegment( return { status: PENDING, id: -1, // lazily assigned later + name: ((Math.random() * 100) | 0).toString(16), index, parentFlushed: false, chunks: [], children: [], formatContext, boundary, - currentEdge, - followingEdge: null, + preEdge: currentEdge, + leadEdge: UNKNOWN_EDGE, + currentEdge: UNKNOWN_EDGE, + postEdge: UNKNOWN_EDGE, }; } @@ -489,7 +492,6 @@ function renderSuspenseBoundary( // use a NODE_EDGE here NODE_EDGE, ); - boundarySegment.followingEdge = NODE_EDGE; parentSegment.children.push(boundarySegment); // This segment is the actual child content. We can start rendering that immediately. @@ -502,7 +504,6 @@ function renderSuspenseBoundary( // use a NODE_EDGE here NODE_EDGE, ); - contentRootSegment.followingEdge = NODE_EDGE; // 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. contentRootSegment.parentFlushed = true; @@ -588,6 +589,12 @@ function renderHostElement( ): void { pushBuiltInComponentStackInDEV(task, type); const segment = task.blockedSegment; + let immediatelyPrecedingChildSegment = + segment.children.length > 0 && + segment.children[segment.children.length - 1].index === + segment.chunks.length + ? segment.children[segment.children.length - 1] + : null; const children = pushStartInstance( segment.chunks, type, @@ -595,8 +602,11 @@ function renderHostElement( request.responseState, segment.formatContext, ); - if (typeof segment.currentEdge === 'object') { - segment.currentEdge.followingEdge = NODE_EDGE; + if (segment.leadEdge === UNKNOWN_EDGE) { + segment.leadEdge = NODE_EDGE; + } + if (immediatelyPrecedingChildSegment) { + immediatelyPrecedingChildSegment.postEdge = NODE_EDGE; } segment.currentEdge = NODE_EDGE; const prevContext = segment.formatContext; @@ -608,11 +618,16 @@ function renderHostElement( // 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); - - if (typeof segment.currentEdge === 'object') { - segment.currentEdge.followingEdge = NODE_EDGE; + immediatelyPrecedingChildSegment = + segment.children.length > 0 && + segment.children[segment.children.length - 1].index === + segment.chunks.length + ? segment.children[segment.children.length - 1] + : null; + if (immediatelyPrecedingChildSegment) { + immediatelyPrecedingChildSegment.postEdge = NODE_EDGE; } + pushEndInstance(segment.chunks, type, props); segment.currentEdge = NODE_EDGE; popComponentStackInDEV(task); } @@ -1261,6 +1276,12 @@ function renderNodeDestructive( if (typeof node === 'string') { const segment = task.blockedSegment; + const immediatelyPrecedingChildSegment = + segment.children.length > 0 && + segment.children[segment.children.length - 1].index === + segment.chunks.length + ? segment.children[segment.children.length - 1] + : null; if ( pushTextInstance( task.blockedSegment.chunks, @@ -1269,8 +1290,11 @@ function renderNodeDestructive( segment.currentEdge === TEXT_EDGE, ) ) { - if (typeof segment.currentEdge === 'object') { - segment.currentEdge.followingEdge = TEXT_EDGE; + if (segment.leadEdge === UNKNOWN_EDGE) { + segment.leadEdge = TEXT_EDGE; + } + if (immediatelyPrecedingChildSegment) { + immediatelyPrecedingChildSegment.postEdge = TEXT_EDGE; } segment.currentEdge = TEXT_EDGE; } @@ -1279,6 +1303,12 @@ function renderNodeDestructive( if (typeof node === 'number') { const segment = task.blockedSegment; + const immediatelyPrecedingChildSegment = + segment.children.length > 0 && + segment.children[segment.children.length - 1].index === + segment.chunks.length + ? segment.children[segment.children.length - 1] + : null; if ( pushTextInstance( task.blockedSegment.chunks, @@ -1287,8 +1317,11 @@ function renderNodeDestructive( segment.currentEdge === TEXT_EDGE, ) ) { - if (typeof segment.currentEdge === 'object') { - segment.currentEdge.followingEdge = TEXT_EDGE; + if (segment.leadEdge === UNKNOWN_EDGE) { + segment.leadEdge = TEXT_EDGE; + } + if (immediatelyPrecedingChildSegment) { + immediatelyPrecedingChildSegment.postEdge = TEXT_EDGE; } segment.currentEdge = TEXT_EDGE; } @@ -1337,11 +1370,10 @@ function spawnNewSuspendedTask( // Adopt the parent segment's leading text embed segment.currentEdge, ); - // Advance the currentEdge to point to this new Segment. - if (typeof segment.currentEdge === 'object') { - segment.currentEdge.followingEdge = newSegment; + if (segment.leadEdge === UNKNOWN_EDGE) { + segment.leadEdge = SEGMENT_EDGE; } - segment.currentEdge = newSegment; + segment.currentEdge = SEGMENT_EDGE; segment.children.push(newSegment); const newTask = createTask( request, @@ -1701,7 +1733,6 @@ function flushSubtree( // The currentEdge of a Pending Segment is the Edge just before the Segment begins because we have not // retried the task for this segment yet. segment.currentEdge = NODE_EDGE; - segment.followingEdge = NODE_EDGE; return writePlaceholder(destination, request.responseState, segmentID); } case COMPLETED: { @@ -1716,22 +1747,7 @@ function flushSubtree( for (; chunkIdx < nextChild.index; chunkIdx++) { writeChunk(destination, chunks[chunkIdx]); } - r = flushSegment(request, destination, nextChild); - if (nextChild.currentEdge === TEXT_EDGE) { - let followingEdge = nextChild.followingEdge; - // Loop over followingEdges that are Segments until you find somethign that isn't a Completed Segment - // The reason we can halt at pending segments is we know they will emit a placeholder and thus act - // like NODE_EDGE - while (followingEdge !== null && typeof followingEdge === 'object') { - if (followingEdge.status === PENDING) { - break; - } - followingEdge = followingEdge.followingEdge; - } - if (followingEdge === TEXT_EDGE) { - r = writeTextSeparator(destination, request.responseState); - } - } + r = flushSegmentInline(request, destination, nextChild); } // Finally just write all the remaining chunks for (; chunkIdx < chunks.length - 1; chunkIdx++) { @@ -1750,16 +1766,61 @@ function flushSubtree( } } +let lastText = false; + function flushSegment( request: Request, destination, segment: Segment, +): boolean { + lastText = false; + return flushSegmentInline(request, destination, segment); +} + +function flushSegmentInline( + request: Request, + destination, + segment: Segment, ): boolean { const boundary = segment.boundary; if (boundary === null) { // Not a suspense boundary. - return flushSubtree(request, destination, segment); - } + + // If parent not flushed we are flushing inside another segment and need to consider text separator + // insertion. If parent is flushed we are a top-level Segment that will be emitted inside non-text + // nodes and can avoid separators regardless of what edges the segment derived when it was rendering + const parentFlushed = segment.parentFlushed; + + // We might have emitted Text or Nodes since the last segment to flush. set the value accordingly or + // retain the existing value if no Edges were emitted + let precedingEdge = segment.preEdge; + lastText = + precedingEdge === TEXT_EDGE + ? true + : precedingEdge === NODE_EDGE + ? false + : lastText; + if (parentFlushed === false && lastText && segment.leadEdge === TEXT_EDGE) { + // This Segment has a Text embedding at the leading edge and we need a separator + writeTextSeparator(destination, request.responseState); + } + let r = flushSubtree(request, destination, segment); + + // We might have emitted Text or Nodes following this segment. set the value accordingly or + // retain teh existing valeu if no Edges were emitted + let lastEdge = segment.currentEdge; + lastText = + lastEdge === TEXT_EDGE ? true : lastEdge === NODE_EDGE ? false : lastText; + if (parentFlushed === false && lastText && segment.postEdge === TEXT_EDGE) { + // This Segment has a Text embedding at the trailing edge and we need a separator + r = writeTextSeparator(destination, request.responseState); + } + + return r; + } + // Boundaries will always write non-Text last + lastText = false; + boundary.parentFlushed = true; // This segment is a Suspense boundary. We need to decide whether to // emit the content or the fallback now. @@ -1833,7 +1894,7 @@ function flushSegment( } const contentSegment = completedSegments[0]; - flushSegment(request, destination, contentSegment); + flushSegmentInline(request, destination, contentSegment); return writeEndCompletedSuspenseBoundary( destination, From e68ac36798923e4910c3e7d79171c3eefcaa9aaa Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 31 May 2022 14:10:50 -0700 Subject: [PATCH 4/4] lints and flow --- packages/react-server/src/ReactFizzServer.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index aa1e12c28f07b..8780f12e48dbf 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -179,7 +179,7 @@ type Segment = { // used to discern when text separator boundaries are needed preEdge: SegmentEdge, leadEdge: SegmentEdge, - trailEdge: SegmentEdge, + currentEdge: SegmentEdge, postEdge: SegmentEdge, }; @@ -1793,7 +1793,7 @@ function flushSegmentInline( // We might have emitted Text or Nodes since the last segment to flush. set the value accordingly or // retain the existing value if no Edges were emitted - let precedingEdge = segment.preEdge; + const precedingEdge = segment.preEdge; lastText = precedingEdge === TEXT_EDGE ? true @@ -1808,7 +1808,7 @@ function flushSegmentInline( // We might have emitted Text or Nodes following this segment. set the value accordingly or // retain teh existing valeu if no Edges were emitted - let lastEdge = segment.currentEdge; + const lastEdge = segment.currentEdge; lastText = lastEdge === TEXT_EDGE ? true : lastEdge === NODE_EDGE ? false : lastText; if (parentFlushed === false && lastText && segment.postEdge === TEXT_EDGE) {