From 7af755e2647b8d944917b2b69f85381fc1bcb033 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 8 Jan 2024 21:34:25 -0500 Subject: [PATCH 1/2] Add test for erroring after having already postponed a previous sibling This can trigger errors due to invalid injected scripts but doesn't actually fail anything else. It's a noop. --- .../src/__tests__/ReactDOMFizzServer-test.js | 135 ++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index eb739eb00ba59..d0dcb2f714743 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -7178,6 +7178,141 @@ describe('ReactDOMFizzServer', () => { ); }); + // @gate enablePostpone + it('can client render a boundary after having already postponed', async () => { + let prerendering = true; + let ssr = true; + + function Postpone() { + if (prerendering) { + React.unstable_postpone(); + } + return 'Hello'; + } + + function ServerError() { + if (ssr) { + throw new Error('server error'); + } + return 'World'; + } + + function App() { + return ( +
+ + + + + + + +
+ ); + } + + const prerenderErrors = []; + const prerendered = await ReactDOMFizzStatic.prerenderToNodeStream( + , + { + onError(x) { + prerenderErrors.push(x.message); + }, + }, + ); + expect(prerendered.postponed).not.toBe(null); + + prerendering = false; + + const ssrErrors = []; + + const resumed = ReactDOMFizzServer.resumeToPipeableStream( + , + JSON.parse(JSON.stringify(prerendered.postponed)), + { + onError(x) { + ssrErrors.push(x.message); + }, + }, + ); + + const windowErrors = []; + function globalError(e) { + windowErrors.push(e.message); + } + window.addEventListener('error', globalError); + + // Create a separate stream so it doesn't close the writable. I.e. simple concat. + const preludeWritable = new Stream.PassThrough(); + preludeWritable.setEncoding('utf8'); + preludeWritable.on('data', chunk => { + writable.write(chunk); + }); + + await act(() => { + prerendered.prelude.pipe(preludeWritable); + }); + + expect(windowErrors).toEqual([]); + + expect(getVisibleChildren(container)).toEqual( +
+ {'Loading1'} + {'Loading2'} +
, + ); + + await act(() => { + resumed.pipe(writable); + }); + + expect(prerenderErrors).toEqual(['server error']); + + // Since this errored, we shouldn't have to replay it. + expect(ssrErrors).toEqual([]); + + expect(windowErrors).toEqual([]); + + // Still loading... + expect(getVisibleChildren(container)).toEqual( +
+ {'Loading1'} + {'Hello'} +
, + ); + + const recoverableErrors = []; + + ssr = false; + + await clientAct(() => { + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(x) { + recoverableErrors.push(x.message); + }, + }); + }); + + expect(recoverableErrors).toEqual( + __DEV__ + ? ['server error'] + : [ + 'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.', + ], + ); + expect(getVisibleChildren(container)).toEqual( +
+ {'Hello'} + {'World'} + {'Hello'} +
, + ); + + expect(windowErrors).toEqual([]); + + window.removeEventListener('error', globalError); + }); + // @gate enablePostpone it('can postpone in fallback', async () => { let prerendering = true; From 01d4d7aac94b008d21742c02908a9e62e4684a12 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 8 Jan 2024 22:35:31 -0500 Subject: [PATCH 2/2] Untrack a boundary after it errors If we have already added resumable slots to a boundary we need to undo that when it errors later. Ideally, we'd remove the whole parent path in this case. If this is the only thing that postpones, we incorrectly think there's resumable slots. --- packages/react-server/src/ReactFizzServer.js | 33 ++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index fe9d7b283b2db..16357649bbb3d 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -994,6 +994,8 @@ function renderSuspenseBoundary( } encodeErrorForBoundary(newBoundary, errorDigest, error, thrownInfo); + untrackBoundary(request, newBoundary); + // We don't need to decrement any task numbers because we didn't spawn any new task. // We don't need to schedule any task because we know the parent has written yet. // We do need to fallthrough to create the fallback though. @@ -2672,6 +2674,34 @@ function trackPostpone( } } +// In case a boundary errors, we need to stop tracking it because we won't +// resume it. +function untrackBoundary(request: Request, boundary: SuspenseBoundary) { + const trackedPostpones = request.trackedPostpones; + if (trackedPostpones === null) { + return; + } + const boundaryKeyPath = boundary.trackedContentKeyPath; + if (boundaryKeyPath === null) { + return; + } + const boundaryNode: void | ReplayNode = + trackedPostpones.workingMap.get(boundaryKeyPath); + if (boundaryNode === undefined) { + return; + } + + // Downgrade to plain ReplayNode since we won't replay through it. + // $FlowFixMe[cannot-write]: We intentionally downgrade this to the other tuple. + boundaryNode.length = 4; + // Remove any resumable slots. + boundaryNode[2] = []; + boundaryNode[3] = null; + + // TODO: We should really just remove the boundary from all parent paths too so + // we don't replay the path to it. +} + function injectPostponedHole( request: Request, task: RenderTask, @@ -3007,6 +3037,7 @@ function erroredTask( if (boundary.status !== CLIENT_RENDERED) { boundary.status = CLIENT_RENDERED; encodeErrorForBoundary(boundary, errorDigest, error, errorInfo); + untrackBoundary(request, boundary); // Regardless of what happens next, this boundary won't be displayed, // so we can flush it, if the parent already flushed. @@ -3192,6 +3223,8 @@ function abortTask(task: Task, request: Request, error: mixed): void { } encodeErrorForBoundary(boundary, errorDigest, errorMessage, errorInfo); + untrackBoundary(request, boundary); + if (boundary.parentFlushed) { request.clientRenderedBoundaries.push(boundary); }