[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 <josh.c.story@gmail.com>
Sebastian Markbåge committed
Sep 29, 2023 at 18:24 UTC
a6ed60a8eb0626e5f84d0bdbb62c0b61219150d3
2 files changed
+132
-121
packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
+96
@@ -6988,4 +6988,100 @@ describe('ReactDOMFizzServer', () => {
6988
</div>,
6989
);
6990
});
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
+ });
7087
});
packages/react-server/src/ReactFizzServer.js
+36
-121
@@ -487,7 +487,7 @@ export function resumeRequest(
487
progressiveChunkSize: postponedState.progressiveChunkSize,
488
status: OPEN,
489
fatalError: null,
490
- nextSegmentId: 0,
490
+ nextSegmentId: postponedState.nextSegmentId,
491
allPendingTasks: 0,
492
pendingRootTasks: 0,
493
completedRootSegment: null,
@@ -1019,11 +1019,7 @@ function replaySuspenseBoundary(
1019
}
1020
try {
1021
// 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);
1023
if (task.replay.pendingTasks === 1 && task.replay.nodes.length > 0) {
1024
throw new Error(
1025
"Couldn't find all resumable slots by key/index during replaying. " +
@@ -1086,56 +1082,27 @@ function replaySuspenseBoundary(
1082
1083
const fallbackKeyPath = [keyPath[0], 'Suspense Fallback', keyPath[2]];
1084
1089
- let suspendedFallbackTask;
1085
// We create suspended task for the fallback because we don't want to actually work
1086
// 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
+ );
1106
if (__DEV__) {
1107
suspendedFallbackTask.componentStack = task.componentStack;
1108
}
@@ -1965,50 +1932,6 @@ function resumeNode(
1932
}
1933
}
1934
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
-
1935
function replayElement(
1936
request: Request,
1937
task: ReplayTask,
@@ -2045,29 +1968,15 @@ function replayElement(
1968
const childSlots = node[3];
1969
task.replay = {nodes: childNodes, slots: childSlots, pendingTasks: 1};
1970
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
+ );
1980
if (
1981
task.replay.pendingTasks === 1 &&
1982
task.replay.nodes.length > 0
@@ -2215,6 +2124,12 @@ function renderNodeDestructiveImpl(
2124
node: ReactNodeList,
2125
childIndex: number,
2126
): 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
+ }
2133
// Stash the node we're working on. We'll pick up from this task in case
2134
// something suspends.
2135
task.node = node;