[Fizz] Fix Client Render after Postpone (#27905)
If we end up client rendering a boundary due to an error after we have already injected a postponed hole in that boundary we'll end up trying to target a missing segment. Since we never insert segments for an already errored boundary into the HTML. Normally an errored prerender wouldn't be used but if it is, such as if it was an intentional client error it triggers this case. Those should really be replaced with postpones though. This is a bit annoying since we eagerly build up the postponed path. I took the easy route here and just cleared out the suspense boundary itself from having any postponed slots. However, this still creates an unnecessary replay path along the way to the boundary. We could probably walk the path and remove any empty parent nodes. What is worse is that if this is the only thing that postponed, we'd still generate a postponed state even though there's actually nothing to resume. Since this is a bit of an edge case already maybe it's fine. In my test I added a check for the `error` event on `window` since this error only surfaces by throwing an ignored error. We should really do that globally for all tests. Our tests should fail by default if there's an error logged to the window.
Sebastian Markbåge committed
Jan 8, 2024 at 23:52 UTC
f9dddcbbb1c0b73f974e78b9488927b778630682
2 files changed
+168
packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
+135
@@ -7178,6 +7178,141 @@ describe('ReactDOMFizzServer', () => {
7178
);
7179
});
7180
7181
+ // @gate enablePostpone
7182
+ it('can client render a boundary after having already postponed', async () => {
7183
+ let prerendering = true;
7184
+ let ssr = true;
7185
+
7186
+ function Postpone() {
7187
+ if (prerendering) {
7188
+ React.unstable_postpone();
7189
+ }
7190
+ return 'Hello';
7191
+ }
7192
+
7193
+ function ServerError() {
7194
+ if (ssr) {
7195
+ throw new Error('server error');
7196
+ }
7197
+ return 'World';
7198
+ }
7199
+
7200
+ function App() {
7201
+ return (
7202
+ <div>
7203
+ <Suspense fallback="Loading1">
7204
+ <Postpone />
7205
+ <ServerError />
7206
+ </Suspense>
7207
+ <Suspense fallback="Loading2">
7208
+ <Postpone />
7209
+ </Suspense>
7210
+ </div>
7211
+ );
7212
+ }
7213
+
7214
+ const prerenderErrors = [];
7215
+ const prerendered = await ReactDOMFizzStatic.prerenderToNodeStream(
7216
+ <App />,
7217
+ {
7218
+ onError(x) {
7219
+ prerenderErrors.push(x.message);
7220
+ },
7221
+ },
7222
+ );
7223
+ expect(prerendered.postponed).not.toBe(null);
7224
+
7225
+ prerendering = false;
7226
+
7227
+ const ssrErrors = [];
7228
+
7229
+ const resumed = ReactDOMFizzServer.resumeToPipeableStream(
7230
+ <App />,
7231
+ JSON.parse(JSON.stringify(prerendered.postponed)),
7232
+ {
7233
+ onError(x) {
7234
+ ssrErrors.push(x.message);
7235
+ },
7236
+ },
7237
+ );
7238
+
7239
+ const windowErrors = [];
7240
+ function globalError(e) {
7241
+ windowErrors.push(e.message);
7242
+ }
7243
+ window.addEventListener('error', globalError);
7244
+
7245
+ // Create a separate stream so it doesn't close the writable. I.e. simple concat.
7246
+ const preludeWritable = new Stream.PassThrough();
7247
+ preludeWritable.setEncoding('utf8');
7248
+ preludeWritable.on('data', chunk => {
7249
+ writable.write(chunk);
7250
+ });
7251
+
7252
+ await act(() => {
7253
+ prerendered.prelude.pipe(preludeWritable);
7254
+ });
7255
+
7256
+ expect(windowErrors).toEqual([]);
7257
+
7258
+ expect(getVisibleChildren(container)).toEqual(
7259
+ <div>
7260
+ {'Loading1'}
7261
+ {'Loading2'}
7262
+ </div>,
7263
+ );
7264
+
7265
+ await act(() => {
7266
+ resumed.pipe(writable);
7267
+ });
7268
+
7269
+ expect(prerenderErrors).toEqual(['server error']);
7270
+
7271
+ // Since this errored, we shouldn't have to replay it.
7272
+ expect(ssrErrors).toEqual([]);
7273
+
7274
+ expect(windowErrors).toEqual([]);
7275
+
7276
+ // Still loading...
7277
+ expect(getVisibleChildren(container)).toEqual(
7278
+ <div>
7279
+ {'Loading1'}
7280
+ {'Hello'}
7281
+ </div>,
7282
+ );
7283
+
7284
+ const recoverableErrors = [];
7285
+
7286
+ ssr = false;
7287
+
7288
+ await clientAct(() => {
7289
+ ReactDOMClient.hydrateRoot(container, <App />, {
7290
+ onRecoverableError(x) {
7291
+ recoverableErrors.push(x.message);
7292
+ },
7293
+ });
7294
+ });
7295
+
7296
+ expect(recoverableErrors).toEqual(
7297
+ __DEV__
7298
+ ? ['server error']
7299
+ : [
7300
+ 'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.',
7301
+ ],
7302
+ );
7303
+ expect(getVisibleChildren(container)).toEqual(
7304
+ <div>
7305
+ {'Hello'}
7306
+ {'World'}
7307
+ {'Hello'}
7308
+ </div>,
7309
+ );
7310
+
7311
+ expect(windowErrors).toEqual([]);
7312
+
7313
+ window.removeEventListener('error', globalError);
7314
+ });
7315
+
7316
// @gate enablePostpone
7317
it('can postpone in fallback', async () => {
7318
let prerendering = true;
packages/react-server/src/ReactFizzServer.js
+33
@@ -994,6 +994,8 @@ function renderSuspenseBoundary(
994
}
995
encodeErrorForBoundary(newBoundary, errorDigest, error, thrownInfo);
996
997
+ untrackBoundary(request, newBoundary);
998
+
999
// We don't need to decrement any task numbers because we didn't spawn any new task.
1000
// We don't need to schedule any task because we know the parent has written yet.
1001
// We do need to fallthrough to create the fallback though.
@@ -2672,6 +2674,34 @@ function trackPostpone(
2674
}
2675
}
2676
2677
+// In case a boundary errors, we need to stop tracking it because we won't
2678
+// resume it.
2679
+function untrackBoundary(request: Request, boundary: SuspenseBoundary) {
2680
+ const trackedPostpones = request.trackedPostpones;
2681
+ if (trackedPostpones === null) {
2682
+ return;
2683
+ }
2684
+ const boundaryKeyPath = boundary.trackedContentKeyPath;
2685
+ if (boundaryKeyPath === null) {
2686
+ return;
2687
+ }
2688
+ const boundaryNode: void | ReplayNode =
2689
+ trackedPostpones.workingMap.get(boundaryKeyPath);
2690
+ if (boundaryNode === undefined) {
2691
+ return;
2692
+ }
2693
+
2694
+ // Downgrade to plain ReplayNode since we won't replay through it.
2695
+ // $FlowFixMe[cannot-write]: We intentionally downgrade this to the other tuple.
2696
+ boundaryNode.length = 4;
2697
+ // Remove any resumable slots.
2698
+ boundaryNode[2] = [];
2699
+ boundaryNode[3] = null;
2700
+
2701
+ // TODO: We should really just remove the boundary from all parent paths too so
2702
+ // we don't replay the path to it.
2703
+}
2704
+
2705
function injectPostponedHole(
2706
request: Request,
2707
task: RenderTask,
@@ -3007,6 +3037,7 @@ function erroredTask(
3037
if (boundary.status !== CLIENT_RENDERED) {
3038
boundary.status = CLIENT_RENDERED;
3039
encodeErrorForBoundary(boundary, errorDigest, error, errorInfo);
3040
+ untrackBoundary(request, boundary);
3041
3042
// Regardless of what happens next, this boundary won't be displayed,
3043
// so we can flush it, if the parent already flushed.
@@ -3192,6 +3223,8 @@ function abortTask(task: Task, request: Request, error: mixed): void {
3223
}
3224
encodeErrorForBoundary(boundary, errorDigest, errorMessage, errorInfo);
3225
3226
+ untrackBoundary(request, boundary);
3227
+
3228
if (boundary.parentFlushed) {
3229
request.clientRenderedBoundaries.push(boundary);
3230
}