Skip to content

Commit

Permalink
[Fizz] Fix for failing id overwrites for postpone (#27684)
Browse files Browse the repository at this point in the history
When we postpone during a render we inject a new segment synchronously
which we postpone. That gets assigned an ID so we can refer to it
immediately in the postponed state.

When we do that, the parent segment may complete later even though it's
also synchronous. If that ends up not having any content in it, it'll
inline into the child and that will override the child's segment id
which is not correct since it was already assigned one.

To fix this, we simply opt-out of the optimization in that case which is
unfortunate because we'll generate many more unnecessary empty segments.
So we should come up with a new strategy for segment id assignment but
this fixes the bug.

Co-authored-by: Josh Story <[email protected]>
  • Loading branch information
sebmarkbage and gnoff authored Nov 10, 2023
1 parent 78c71bc commit 0e352ea
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 5 deletions.
47 changes: 47 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1495,4 +1495,51 @@ describe('ReactDOMFizzStaticBrowser', () => {
'hello',
]);
});

// @gate enablePostpone
it('can render a deep list of single components where one postpones', async () => {
let isPrerendering = true;
function Outer({children}) {
return children;
}

function Middle({children}) {
return children;
}

function Inner() {
if (isPrerendering) {
React.unstable_postpone();
}
return 'hello';
}

function App() {
return (
<Suspense fallback="loading...">
<Outer>
<Middle>
<Inner />
</Middle>
</Outer>
</Suspense>
);
}

const prerendered = await ReactDOMFizzStatic.prerender(<App />);
const postponedState = JSON.stringify(prerendered.postponed);

await readIntoContainer(prerendered.prelude);
expect(getVisibleChildren(container)).toEqual('loading...');

isPrerendering = false;

const dynamic = await ReactDOMFizzServer.resume(
<App />,
JSON.parse(postponedState),
);

await readIntoContainer(dynamic);
expect(getVisibleChildren(container)).toEqual('hello');
});
});
18 changes: 13 additions & 5 deletions packages/react-server/src/ReactFizzServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2542,15 +2542,22 @@ function trackPostpone(

const children: Array<ReplayNode> = [];
if (boundaryKeyPath === keyPath && task.childIndex === -1) {
// Since we postponed directly in the Suspense boundary we can't have written anything
// to its segment. Therefore this will end up becoming the root segment.
segment.id = boundary.rootSegmentID;
// Assign ID
if (segment.id === -1) {
if (segment.parentFlushed) {
// If this segment's parent was already flushed, it means we really just
// skipped the parent and this segment is now the root.
segment.id = boundary.rootSegmentID;
} else {
segment.id = request.nextSegmentId++;
}
}
// We postponed directly inside the Suspense boundary so we mark this for resuming.
const boundaryNode: ReplaySuspenseBoundary = [
boundaryKeyPath[1],
boundaryKeyPath[2],
children,
boundary.rootSegmentID,
segment.id,
fallbackReplayNode,
boundary.rootSegmentID,
];
Expand Down Expand Up @@ -3264,7 +3271,8 @@ function queueCompletedSegment(
if (
segment.chunks.length === 0 &&
segment.children.length === 1 &&
segment.children[0].boundary === null
segment.children[0].boundary === null &&
segment.children[0].id === -1
) {
// This is an empty segment. There's nothing to write, so we can instead transfer the ID
// to the child. That way any existing references point to the child.
Expand Down

0 comments on commit 0e352ea

Please sign in to comment.