From 352f0b65c3d27a84dc9b149d7359779c85706064 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 3 Oct 2023 23:04:50 -0400 Subject: [PATCH 1/3] Reset error component stack and fix error messages --- .../ReactDOMFizzStaticBrowser-test.js | 79 +++++++++++++++++++ packages/react-server/src/ReactFizzServer.js | 34 +++++--- scripts/error-codes/codes.json | 4 +- 3 files changed, 104 insertions(+), 13 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js index 9ec0f2c97c9fb..facfee69ad6f3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js @@ -981,4 +981,83 @@ describe('ReactDOMFizzStaticBrowser', () => { expect(getVisibleChildren(container)).toEqual(
Hello
); }); + + // @gate enablePostpone + it('errors if the replay does not line up', async () => { + let prerendering = true; + function Postpone() { + if (prerendering) { + React.unstable_postpone(); + } + return 'Hello'; + } + + function Wrapper({children}) { + return children; + } + + function App() { + const children = ( + + + + ); + return ( + <> +
{prerendering ? {children} : children}
+
+ {prerendering ? ( + +
+ +
+ + ) : ( + + )} +
+ + ); + } + + const prerendered = await ReactDOMFizzStatic.prerender(); + expect(prerendered.postponed).not.toBe(null); + + await readIntoContainer(prerendered.prelude); + + expect(getVisibleChildren(container)).toEqual([ +
Loading...
, +
Loading...
, + ]); + + prerendering = false; + + const errors = []; + const resumed = await ReactDOMFizzServer.resume( + , + JSON.parse(JSON.stringify(prerendered.postponed)), + { + onError(x) { + errors.push(x.message); + }, + }, + ); + + expect(errors).toEqual([ + 'Expected the resume to render in this slot but instead it rendered . ' + + "The tree doesn't match so React will fallback to client rendering.", + 'Expected the resume to render in this slot but instead it rendered . ' + + "The tree doesn't match so React will fallback to client rendering.", + ]); + + // TODO: Test the component stack but we don't expose it to the server yet. + + await readIntoContainer(resumed); + + // Client rendered + expect(getVisibleChildren(container)).toEqual([ +
Loading...
, +
Loading...
, + ]); + }); }); diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 951b3e07930a2..c44f9a2e6e1f8 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -1110,7 +1110,6 @@ function replaySuspenseBoundary( // on preparing fallbacks if we don't have any more main content to task on. request.pingedTasks.push(suspendedFallbackTask); - // TODO: Should this be in the finally? popComponentStackInDEV(task); } @@ -1953,17 +1952,19 @@ function replayElement( if (keyOrIndex !== node[1]) { continue; } - // Let's double check that the component name matches as a precaution. - if (name !== null && name !== node[0]) { - throw new Error( - 'Expected to see a component of type "' + - name + - '" in this slot. ' + - "The tree doesn't match so React will fallback to client rendering.", - ); - } if (node.length === 4) { // Matched a replayable path. + // Let's double check that the component name matches as a precaution. + if (name !== null && name !== node[0]) { + throw new Error( + 'Expected the resume to render <' + + (node[0]: any) + + '> in this slot but instead it rendered <' + + name + + '>. ' + + "The tree doesn't match so React will fallback to client rendering.", + ); + } const childNodes = node[2]; const childSlots = node[3]; task.replay = {nodes: childNodes, slots: childSlots, pendingTasks: 1}; @@ -2009,8 +2010,13 @@ function replayElement( } else { // Let's double check that the component type matches. if (type !== REACT_SUSPENSE_TYPE) { + const expectedType = 'Suspense'; throw new Error( - 'Expected to see a Suspense boundary in this slot. ' + + 'Expected the resume to render <' + + expectedType + + '> in this slot but instead it rendered <' + + (getComponentNameFromType(type) || 'Unknown') + + '>. ' + "The tree doesn't match so React will fallback to client rendering.", ); } @@ -2378,6 +2384,7 @@ function replayFragment( // in the original prerender. What's unable to complete is the child // replay nodes which might be Suspense boundaries which are able to // absorb the error and we can still continue with siblings. + // This is an error, stash the component stack if it is null. erroredReplay(request, task.blockedBoundary, x, childNodes, childSlots); } finally { task.replay.pendingTasks--; @@ -2849,6 +2856,7 @@ function renderNode( if (__DEV__) { task.componentStack = previousComponentStack; } + lastBoundaryErrorComponentStackDev = null; return; } } @@ -2930,6 +2938,7 @@ function erroredTask( errorDigest = logRecoverableError(request, error); } if (boundary === null) { + lastBoundaryErrorComponentStackDev = null; fatalError(request, error); } else { boundary.pendingTasks--; @@ -2949,6 +2958,8 @@ function erroredTask( // We reuse the same queue for errors. request.clientRenderedBoundaries.push(boundary); } + } else { + lastBoundaryErrorComponentStackDev = null; } } @@ -3365,6 +3376,7 @@ function retryRenderTask( logPostpone(request, postponeInstance.message); trackPostpone(request, trackedPostpones, task, segment); finishedTask(request, task.blockedBoundary, segment); + lastBoundaryErrorComponentStackDev = null; return; } } diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index be62358481bc0..851c058cf1b2d 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -474,8 +474,8 @@ "486": "It should not be possible to postpone at the root. This is a bug in React.", "487": "We should not have any resumable nodes in the shell. This is a bug in React.", "488": "Couldn't find all resumable slots by key/index during replaying. The tree doesn't match so React will fallback to client rendering.", - "489": "Expected to see a component of type \"%s\" in this slot. The tree doesn't match so React will fallback to client rendering.", - "490": "Expected to see a Suspense boundary in this slot. The tree doesn't match so React will fallback to client rendering.", + "489": "Expected the resume to render <%s> in this slot but instead it rendered <%s>. The tree doesn't match so React will fallback to client rendering.", + "490": "Expected the resume to render <%s> in this slot but instead it rendered <%s>. The tree doesn't match so React will fallback to client rendering.", "491": "It should not be possible to postpone both at the root of an element as well as a slot below. This is a bug in React.", "492": "The \"react\" package in this environment is not configured correctly. The \"react-server\" condition must be enabled in any environment that runs React Server Components.", "493": "To taint a value, a lifetime must be defined by passing an object that holds the value.", From 35f1d0f86d27c84a702a0f1417a0c389f5ddc822 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 4 Oct 2023 00:18:23 -0400 Subject: [PATCH 2/3] Resolve shell when a replay task errors --- .../src/__tests__/ReactDOMFizzStaticBrowser-test.js | 7 ++++++- packages/react-server/src/ReactFizzServer.js | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js index facfee69ad6f3..f5cdd6c7450f9 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js @@ -996,6 +996,11 @@ describe('ReactDOMFizzStaticBrowser', () => { return children; } + const lazySpan = React.lazy(async () => { + await 0; + return {default: }; + }); + function App() { const children = ( @@ -1013,7 +1018,7 @@ describe('ReactDOMFizzStaticBrowser', () => { ) : ( - + lazySpan )} diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index c44f9a2e6e1f8..ce2a1b0931f2c 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -3464,6 +3464,12 @@ function retryReplayTask(request: Request, task: ReplayTask): void { task.replay.nodes, task.replay.slots, ); + request.pendingRootTasks--; + if (request.pendingRootTasks === 0) { + request.onShellError = noop; + const onShellReady = request.onShellReady; + onShellReady(); + } request.allPendingTasks--; if (request.allPendingTasks === 0) { const onAllReady = request.onAllReady; From 104995f98abf5e1b5f897962832fc1a0342c0461 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 4 Oct 2023 00:35:28 -0400 Subject: [PATCH 3/3] Aborting a replay task at the root should still complete the shell --- .../ReactDOMFizzStaticBrowser-test.js | 61 +++++++++++++++++++ packages/react-server/src/ReactFizzServer.js | 18 ++++-- 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js index f5cdd6c7450f9..9f8bb2fc07fc5 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js @@ -1065,4 +1065,65 @@ describe('ReactDOMFizzStaticBrowser', () => {
Loading...
, ]); }); + + // @gate enablePostpone + it('can abort the resume', async () => { + let prerendering = true; + const infinitePromise = new Promise(() => {}); + function Postpone() { + if (prerendering) { + React.unstable_postpone(); + } + return 'Hello'; + } + + function App() { + if (!prerendering) { + React.use(infinitePromise); + } + return ( +
+ + + +
+ ); + } + + const prerendered = await ReactDOMFizzStatic.prerender(); + expect(prerendered.postponed).not.toBe(null); + + await readIntoContainer(prerendered.prelude); + + expect(getVisibleChildren(container)).toEqual(
Loading...
); + + prerendering = false; + + const controller = new AbortController(); + + const errors = []; + + const resumedPromise = ReactDOMFizzServer.resume( + , + JSON.parse(JSON.stringify(prerendered.postponed)), + { + signal: controller.signal, + onError(x) { + errors.push(x); + }, + }, + ); + + controller.abort('abort'); + + const resumed = await resumedPromise; + await resumed.allReady; + + expect(errors).toEqual(['abort']); + + await readIntoContainer(resumed); + + // Client rendered + expect(getVisibleChildren(container)).toEqual(
Loading...
); + }); }); diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index ce2a1b0931f2c..00b1c29843599 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -3088,7 +3088,6 @@ function abortTask(task: Task, request: Request, error: mixed): void { } if (boundary === null) { - request.allPendingTasks--; if (request.status !== CLOSING && request.status !== CLOSED) { const replay: null | ReplaySet = task.replay; if (replay === null) { @@ -3096,6 +3095,7 @@ function abortTask(task: Task, request: Request, error: mixed): void { // the request; logRecoverableError(request, error); fatalError(request, error); + return; } else { // If the shell aborts during a replay, that's not a fatal error. Instead // we should be able to recover by client rendering all the root boundaries in @@ -3112,6 +3112,12 @@ function abortTask(task: Task, request: Request, error: mixed): void { errorDigest, ); } + request.pendingRootTasks--; + if (request.pendingRootTasks === 0) { + request.onShellError = noop; + const onShellReady = request.onShellReady; + onShellReady(); + } } } } else { @@ -3148,12 +3154,12 @@ function abortTask(task: Task, request: Request, error: mixed): void { abortTask(fallbackTask, request, error), ); boundary.fallbackAbortableTasks.clear(); + } - request.allPendingTasks--; - if (request.allPendingTasks === 0) { - const onAllReady = request.onAllReady; - onAllReady(); - } + request.allPendingTasks--; + if (request.allPendingTasks === 0) { + const onAllReady = request.onAllReady; + onAllReady(); } }