Skip to content

Commit a6ed60a

Browse files
sebmarkbagegnoff
sebmarkbage
and
gnoff
authored
[Fizz] Don't double replay elements when it's the postpone point (#27440)
The `resumeElement` function wasn't actually doing the correct thing because it was resuming the element itself but what the child slot means is that we're supposed to resume in the direct child of the element. This is difficult to check for since it's all the possible branches that the element can render into, so instead we just check this in renderNode. It means the hottest path always checks the task which is unfortunate. And also, resuming using the correct nextSegmentId. Fixes two bugs surfaced by this test. --------- Co-authored-by: Josh Story <[email protected]>
1 parent c7ba8c0 commit a6ed60a

File tree

2 files changed

+132
-121
lines changed

2 files changed

+132
-121
lines changed

‎packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6988,4 +6988,100 @@ describe('ReactDOMFizzServer', () => {
69886988
</div>,
69896989
);
69906990
});
6991+
6992+
// @gate enablePostpone
6993+
it('can discover new suspense boundaries in the resume', async () => {
6994+
let prerendering = true;
6995+
let resolveA;
6996+
const promiseA = new Promise(r => (resolveA = r));
6997+
let resolveB;
6998+
const promiseB = new Promise(r => (resolveB = r));
6999+
7000+
function WaitA() {
7001+
return React.use(promiseA);
7002+
}
7003+
function WaitB() {
7004+
return React.use(promiseB);
7005+
}
7006+
function Postpone() {
7007+
if (prerendering) {
7008+
React.unstable_postpone();
7009+
}
7010+
return (
7011+
<span>
7012+
<Suspense fallback="Loading again...">
7013+
<WaitA />
7014+
</Suspense>
7015+
<WaitB />
7016+
</span>
7017+
);
7018+
}
7019+
7020+
function App() {
7021+
return (
7022+
<div>
7023+
<Suspense fallback="Loading...">
7024+
<p>
7025+
<Postpone />
7026+
</p>
7027+
</Suspense>
7028+
</div>
7029+
);
7030+
}
7031+
7032+
const prerendered = await ReactDOMFizzStatic.prerenderToNodeStream(<App />);
7033+
expect(prerendered.postponed).not.toBe(null);
7034+
7035+
prerendering = false;
7036+
7037+
// Create a separate stream so it doesn't close the writable. I.e. simple concat.
7038+
const preludeWritable = new Stream.PassThrough();
7039+
preludeWritable.setEncoding('utf8');
7040+
preludeWritable.on('data', chunk => {
7041+
writable.write(chunk);
7042+
});
7043+
7044+
await act(() => {
7045+
prerendered.prelude.pipe(preludeWritable);
7046+
});
7047+
7048+
const resumed = await ReactDOMFizzServer.resumeToPipeableStream(
7049+
<App />,
7050+
JSON.parse(JSON.stringify(prerendered.postponed)),
7051+
);
7052+
7053+
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
7054+
7055+
// Read what we've completed so far
7056+
await act(() => {
7057+
resumed.pipe(writable);
7058+
});
7059+
7060+
// Still blocked
7061+
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
7062+
7063+
// Resolve the first promise, this unblocks the inner boundary
7064+
await act(() => {
7065+
resolveA('Hello');
7066+
});
7067+
7068+
// Still blocked
7069+
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
7070+
7071+
// Resolve the second promise, this unblocks the outer boundary
7072+
await act(() => {
7073+
resolveB('World');
7074+
});
7075+
7076+
expect(getVisibleChildren(container)).toEqual(
7077+
<div>
7078+
<p>
7079+
<span>
7080+
{'Hello'}
7081+
{'World'}
7082+
</span>
7083+
</p>
7084+
</div>,
7085+
);
7086+
});
69917087
});

‎packages/react-server/src/ReactFizzServer.js‎

Lines changed: 36 additions & 121 deletions
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,7 @@ export function resumeRequest(
487487
progressiveChunkSize: postponedState.progressiveChunkSize,
488488
status: OPEN,
489489
fatalError: null,
490-
nextSegmentId: 0,
490+
nextSegmentId: postponedState.nextSegmentId,
491491
allPendingTasks: 0,
492492
pendingRootTasks: 0,
493493
completedRootSegment: null,
@@ -1019,11 +1019,7 @@ function replaySuspenseBoundary(
10191019
}
10201020
try {
10211021
// We use the safe form because we don't handle suspending here. Only error handling.
1022-
if (typeof childSlots === 'number') {
1023-
resumeNode(request, task, childSlots, content, -1);
1024-
} else {
1025-
renderNode(request, task, content, -1);
1026-
}
1022+
renderNode(request, task, content, -1);
10271023
if (task.replay.pendingTasks === 1 && task.replay.nodes.length > 0) {
10281024
throw new Error(
10291025
"Couldn't find all resumable slots by key/index during replaying. " +
@@ -1086,56 +1082,27 @@ function replaySuspenseBoundary(
10861082

10871083
const fallbackKeyPath = [keyPath[0], 'Suspense Fallback', keyPath[2]];
10881084

1089-
let suspendedFallbackTask;
10901085
// We create suspended task for the fallback because we don't want to actually work
10911086
// on it yet in case we finish the main content, so we queue for later.
1092-
if (typeof fallbackSlots === 'number') {
1093-
// Resuming directly in the fallback.
1094-
const resumedSegment = createPendingSegment(
1095-
request,
1096-
0,
1097-
null,
1098-
task.formatContext,
1099-
false,
1100-
false,
1101-
);
1102-
resumedSegment.id = fallbackSlots;
1103-
resumedSegment.parentFlushed = true;
1104-
suspendedFallbackTask = createRenderTask(
1105-
request,
1106-
null,
1107-
fallback,
1108-
-1,
1109-
parentBoundary,
1110-
resumedSegment,
1111-
fallbackAbortSet,
1112-
fallbackKeyPath,
1113-
task.formatContext,
1114-
task.legacyContext,
1115-
task.context,
1116-
task.treeContext,
1117-
);
1118-
} else {
1119-
const fallbackReplay = {
1120-
nodes: fallbackNodes,
1121-
slots: fallbackSlots,
1122-
pendingTasks: 0,
1123-
};
1124-
suspendedFallbackTask = createReplayTask(
1125-
request,
1126-
null,
1127-
fallbackReplay,
1128-
fallback,
1129-
-1,
1130-
parentBoundary,
1131-
fallbackAbortSet,
1132-
fallbackKeyPath,
1133-
task.formatContext,
1134-
task.legacyContext,
1135-
task.context,
1136-
task.treeContext,
1137-
);
1138-
}
1087+
const fallbackReplay = {
1088+
nodes: fallbackNodes,
1089+
slots: fallbackSlots,
1090+
pendingTasks: 0,
1091+
};
1092+
const suspendedFallbackTask = createReplayTask(
1093+
request,
1094+
null,
1095+
fallbackReplay,
1096+
fallback,
1097+
-1,
1098+
parentBoundary,
1099+
fallbackAbortSet,
1100+
fallbackKeyPath,
1101+
task.formatContext,
1102+
task.legacyContext,
1103+
task.context,
1104+
task.treeContext,
1105+
);
11391106
if (__DEV__) {
11401107
suspendedFallbackTask.componentStack = task.componentStack;
11411108
}
@@ -1965,50 +1932,6 @@ function resumeNode(
19651932
}
19661933
}
19671934

1968-
function resumeElement(
1969-
request: Request,
1970-
task: ReplayTask,
1971-
keyPath: KeyNode,
1972-
segmentId: number,
1973-
prevThenableState: ThenableState | null,
1974-
type: any,
1975-
props: Object,
1976-
ref: any,
1977-
): void {
1978-
const prevReplay = task.replay;
1979-
const blockedBoundary = task.blockedBoundary;
1980-
const resumedSegment = createPendingSegment(
1981-
request,
1982-
0,
1983-
null,
1984-
task.formatContext,
1985-
false,
1986-
false,
1987-
);
1988-
resumedSegment.id = segmentId;
1989-
resumedSegment.parentFlushed = true;
1990-
try {
1991-
// Convert the current ReplayTask to a RenderTask.
1992-
const renderTask: RenderTask = (task: any);
1993-
renderTask.replay = null;
1994-
renderTask.blockedSegment = resumedSegment;
1995-
renderElement(request, task, keyPath, prevThenableState, type, props, ref);
1996-
resumedSegment.status = COMPLETED;
1997-
if (blockedBoundary === null) {
1998-
request.completedRootSegment = resumedSegment;
1999-
} else {
2000-
queueCompletedSegment(blockedBoundary, resumedSegment);
2001-
if (blockedBoundary.parentFlushed) {
2002-
request.partialBoundaries.push(blockedBoundary);
2003-
}
2004-
}
2005-
} finally {
2006-
// Restore to a ReplayTask.
2007-
task.replay = prevReplay;
2008-
task.blockedSegment = null;
2009-
}
2010-
}
2011-
20121935
function replayElement(
20131936
request: Request,
20141937
task: ReplayTask,
@@ -2045,29 +1968,15 @@ function replayElement(
20451968
const childSlots = node[3];
20461969
task.replay = {nodes: childNodes, slots: childSlots, pendingTasks: 1};
20471970
try {
2048-
if (typeof childSlots === 'number') {
2049-
// Matched a resumable element.
2050-
resumeElement(
2051-
request,
2052-
task,
2053-
keyPath,
2054-
childSlots,
2055-
prevThenableState,
2056-
type,
2057-
props,
2058-
ref,
2059-
);
2060-
} else {
2061-
renderElement(
2062-
request,
2063-
task,
2064-
keyPath,
2065-
prevThenableState,
2066-
type,
2067-
props,
2068-
ref,
2069-
);
2070-
}
1971+
renderElement(
1972+
request,
1973+
task,
1974+
keyPath,
1975+
prevThenableState,
1976+
type,
1977+
props,
1978+
ref,
1979+
);
20711980
if (
20721981
task.replay.pendingTasks === 1 &&
20731982
task.replay.nodes.length > 0
@@ -2215,6 +2124,12 @@ function renderNodeDestructiveImpl(
22152124
node: ReactNodeList,
22162125
childIndex: number,
22172126
): void {
2127+
if (task.replay !== null && typeof task.replay.slots === 'number') {
2128+
// TODO: Figure out a cheaper place than this hot path to do this check.
2129+
const resumeSegmentID = task.replay.slots;
2130+
resumeNode(request, task, resumeSegmentID, node, childIndex);
2131+
return;
2132+
}
22182133
// Stash the node we're working on. We'll pick up from this task in case
22192134
// something suspends.
22202135
task.node = node;

0 commit comments

Comments
 (0)