-
Notifications
You must be signed in to change notification settings - Fork 337
fix(app-router): recover SSR shell render errors via __next_error__ document #1908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 19 commits
2a9a102
99789d7
dab9c72
7f8b8ba
f8ff07c
ad6daac
2a6292a
57b01b4
2da1242
e9f0119
a2f481b
c760b2a
001f36a
8a259f4
fe3df57
e58a313
98b93df
53aef71
384c40e
07990b5
158e082
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -714,6 +714,7 @@ export default __createAppRscHandler({ | |
| getSourceRoute(sourceRouteIndex) { | ||
| return routes[sourceRouteIndex]; | ||
| }, | ||
| hasCustomGlobalError: ${globalErrorVar ? `Boolean(${globalErrorVar}?.default)` : "false"}, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| hasGenerateStaticParams: __generateStaticParams.length > 0, | ||
| hasPageDefaultExport: !!PageComponent, | ||
| hasPageModule: !!route.page, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -190,7 +190,7 @@ import { createRequire } from "node:module"; | |
| import fs from "node:fs"; | ||
| import { randomBytes, randomUUID } from "node:crypto"; | ||
| import commonjs from "vite-plugin-commonjs"; | ||
| import { normalizePathSeparators, stripViteModuleQuery } from "./utils/path.js"; | ||
| import { normalizePathSeparators, stripJsExtension, stripViteModuleQuery } from "./utils/path.js"; | ||
| import { getViteMajorVersion } from "./utils/vite-version.js"; | ||
|
|
||
| // Install the process-level peer-disconnect backstop at module load. | ||
|
|
@@ -2504,7 +2504,7 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] { | |
| // direct @vercel/og imports in metadata routes, and \0-prefixed | ||
| // re-imports from @vitejs/plugin-rsc. | ||
| filter: { | ||
| id: /(?:next\/|virtual:vinext-|^@vercel\/og(?:\.js)?$)/, | ||
| id: /(?:next\/|vinext\/shims\/|virtual:vinext-|@vercel\/og(?:\.js)?$)/, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The meaningful change here besides adding |
||
| }, | ||
| handler(id, importer) { | ||
| // Strip \0 prefix if present — @vitejs/plugin-rsc's generated | ||
|
|
@@ -2517,6 +2517,14 @@ export default function vinext(options: VinextOptions = {}): PluginOption[] { | |
| return resolveShimModulePath(_shimsDir, "og"); | ||
| } | ||
|
|
||
| const vinextShimPrefix = "vinext/shims/"; | ||
| if (cleanId.startsWith(vinextShimPrefix)) { | ||
| return resolveShimModulePath( | ||
| _shimsDir, | ||
| stripJsExtension(stripViteModuleQuery(cleanId.slice(vinextShimPrefix.length))), | ||
| ); | ||
| } | ||
|
|
||
| // Pages Router virtual modules | ||
| if (cleanId === VIRTUAL_SERVER_ENTRY) return RESOLVED_SERVER_ENTRY; | ||
| if (cleanId === VIRTUAL_CLIENT_ENTRY) return RESOLVED_CLIENT_ENTRY; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ import { | |
| setServerCallback, | ||
| } from "@vitejs/plugin-rsc/browser"; | ||
| import { flushSync } from "react-dom"; | ||
| import { hydrateRoot } from "react-dom/client"; | ||
| import { createRoot, hydrateRoot } from "react-dom/client"; | ||
| import "../client/instrumentation-client.js"; | ||
| import { notifyAppRouterTransitionStart } from "../client/instrumentation-client-state.js"; | ||
| import { | ||
|
|
@@ -1647,16 +1647,35 @@ function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void { | |
| onCaughtError: prodOnCaughtError, | ||
| onUncaughtError, | ||
| }); | ||
| window.__VINEXT_RSC_ROOT__ = hydrateRootInTransition({ | ||
| children: createElement(BrowserRoot, { | ||
| initialElements: root, | ||
| initialNavigationSnapshot, | ||
| }), | ||
| container: document, | ||
| hydrateRoot, | ||
| options: hydrateRootOptions, | ||
| startTransition, | ||
| const children = createElement(BrowserRoot, { | ||
| initialElements: root, | ||
| initialNavigationSnapshot, | ||
| }); | ||
| const errorShellStyles = document.querySelectorAll("style[data-vinext-error-shell-style]"); | ||
| if (document.documentElement.id === "__next_error__") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Behavior-change worth confirming: on This is plausibly upstream-compatible (Next.js client-renders the recovery shell), and the embedded flight payload encodes the same error tree so the rendered content matches. But the new browser spec only exercises the SSR-only-throw recovery path, not the legacy server-digest-error path through this same branch. Worth a quick confirmation (or a test) that a genuine server-side digest error with no custom global-error still renders the default error card correctly via |
||
| // Next.js client/app-index.tsx uses the document id alone to select CSR | ||
| // after any failed App Router server render. The style marker only scopes | ||
| // cleanup to vinext's shell-recovery placeholder styles. | ||
| // There is no server-rendered form to hydrate in this client-render path; | ||
| // reuse only the shared root error callbacks and related root options. | ||
| const { formState: _inertFormState, ...createRootOptions } = hydrateRootOptions; | ||
| for (const style of errorShellStyles) { | ||
| style.remove(); | ||
| } | ||
| startTransition(() => { | ||
| const clientRoot = createRoot(document, createRootOptions); | ||
| clientRoot.render(children); | ||
| window.__VINEXT_RSC_ROOT__ = clientRoot; | ||
| }); | ||
| } else { | ||
| window.__VINEXT_RSC_ROOT__ = hydrateRootInTransition({ | ||
| children, | ||
| container: document, | ||
| hydrateRoot, | ||
| options: hydrateRootOptions, | ||
| startTransition, | ||
| }); | ||
| } | ||
| markInitialAppRouterBootstrapHydrated(); | ||
|
|
||
| const navigateRsc: NavigationRuntimeNavigate = async function navigateRsc( | ||
|
|
@@ -2293,6 +2312,9 @@ function bootstrapHydration(rscStream: ReadableStream<Uint8Array>): void { | |
| // original document from there, so let the next HMR update reload the | ||
| // current URL. If the edit fixed the error the page comes back clean; if | ||
| // not, initial dev server errors re-populate the overlay. | ||
| // | ||
| // Reloading is safe for any default-error document because the dev | ||
| // server will render the current state of the source after the edit. | ||
| if (document.documentElement.id === "__next_error__") { | ||
| window.location.reload(); | ||
| return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ import { | |
| } from "./app-page-stream.js"; | ||
| import { AppElementsWire, type AppElements } from "./app-elements.js"; | ||
| import { createAppPageLayoutEntries, createAppPageSourcePage } from "./app-page-route-wiring.js"; | ||
| import { NEVER_CACHE_CONTROL } from "./cache-control.js"; | ||
|
|
||
| // oxlint-disable-next-line @typescript-eslint/no-explicit-any | ||
| type AppPageComponent = ComponentType<any>; | ||
|
|
@@ -500,17 +501,25 @@ export async function renderAppPageErrorBoundary<TModule extends AppPageModule>( | |
| }); | ||
| }; | ||
|
|
||
| const renderWith = (BoundaryComponent: AppPageComponent): Promise<Response> => | ||
| renderAppPageBoundaryElementResponse({ | ||
| const renderWith = async (BoundaryComponent: AppPageComponent): Promise<Response> => { | ||
| const response = await renderAppPageBoundaryElementResponse({ | ||
| ...options, | ||
| element: buildElement(BoundaryComponent), | ||
| initialDevServerError: rawError, | ||
| layoutModules, | ||
| navigationParams: matchedParams, | ||
| route: options.route, | ||
| routePattern: options.route?.pattern, | ||
| status: 200, | ||
| status: errorBoundary.isGlobalError ? 500 : 200, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the unannounced scope expansion (added in The behavior is correct (Next.js returns 500 for unhandled errors that escalate to global-error; the prior 200 was the divergence), so this isn't a blocker. But the PR description lists "changing custom-global-error apps' behavior in any way" as a non-goal and claims
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirming the prior round's observation still stands at The behavior is right (Next.js returns 500 for unhandled errors escalated to global-error; the old 200 was the divergence). But the PR description still lists "changing custom-global-error apps' behavior in any way" as a non-goal and claims |
||
| }); | ||
| if (errorBoundary.isGlobalError) { | ||
| response.headers.set("Cache-Control", NEVER_CACHE_CONTROL); | ||
| response.headers.delete("CDN-Cache-Control"); | ||
| response.headers.delete("Cloudflare-CDN-Cache-Control"); | ||
| response.headers.delete("Cache-Tag"); | ||
| } | ||
| return response; | ||
| }; | ||
|
|
||
| try { | ||
| return await renderWith(errorBoundary.component); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -253,6 +253,7 @@ type DispatchAppPageOptions<TRoute extends AppPageDispatchRoute> = { | |
| getNavigationContext: () => NavigationContext | null; | ||
| getSourceRoute: (sourceRouteIndex: number) => TRoute | undefined; | ||
| hasGenerateStaticParams: boolean; | ||
| hasCustomGlobalError?: boolean; | ||
| hasPageDefaultExport: boolean; | ||
| hasPageModule: boolean; | ||
| handlerStart: number; | ||
|
|
@@ -1136,6 +1137,7 @@ async function dispatchAppPageInner<TRoute extends AppPageDispatchRoute>( | |
| return renderPageSpecialError(options, specialError); | ||
| }, | ||
| renderToReadableStream: options.renderToReadableStream, | ||
| hasCustomGlobalError: options.hasCustomGlobalError, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This threads the flag into the initial render lifecycle. The background-revalidation |
||
| prerenderToReadableStream: options.prerenderToReadableStream, | ||
| routePattern: route.pattern, | ||
| runWithSuppressedHookWarning(probe) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,7 +57,7 @@ import type { | |
| ClientReuseManifestSkipDisposition, | ||
| ClientReuseManifestTraceFields, | ||
| } from "./client-reuse-manifest.js"; | ||
| import { NO_STORE_CACHE_CONTROL } from "./cache-control.js"; | ||
| import { NEVER_CACHE_CONTROL, NO_STORE_CACHE_CONTROL } from "./cache-control.js"; | ||
| import { | ||
| createClientReuseSkipTransportPlan, | ||
| createStaticLayoutClientReuseArtifactCompatibility, | ||
|
|
@@ -127,6 +127,7 @@ type RenderAppPageLifecycleOptions = { | |
| peekRequestCacheLife?: () => AppPageRequestCacheLife | null; | ||
| getDraftModeCookieHeader: () => string | null | undefined; | ||
| handlerStart: number; | ||
| hasCustomGlobalError?: boolean; | ||
| hasLoadingBoundary: boolean; | ||
| dynamicStaleTimeSeconds?: number; | ||
| isDynamicError: boolean; | ||
|
|
@@ -868,6 +869,7 @@ export async function renderAppPageLifecycle( | |
| return renderAppPageHtmlStream({ | ||
| capturedRscDataRef, | ||
| fontData, | ||
| hasCustomGlobalError: options.hasCustomGlobalError, | ||
| navigationContext: options.getNavigationContext(), | ||
| basePath: options.basePath, | ||
| clientTraceMetadata: options.clientTraceMetadata, | ||
|
|
@@ -975,6 +977,25 @@ export async function renderAppPageLifecycle( | |
| responseKind: "html", | ||
| }); | ||
|
|
||
| if (htmlRender.shellErrorRecovered) { | ||
| const response = buildAppPageHtmlResponse(safeHtmlStream, { | ||
| draftCookie, | ||
| linkHeader, | ||
| isEdgeRuntime: options.isEdgeRuntime, | ||
| middlewareContext: { | ||
| headers: options.middlewareContext.headers, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed this is safe: |
||
| status: 500, | ||
| }, | ||
| policy: { cacheControl: NEVER_CACHE_CONTROL }, | ||
| timing: htmlResponseTiming, | ||
| }); | ||
| response.headers.set("Cache-Control", NEVER_CACHE_CONTROL); | ||
| response.headers.delete("CDN-Cache-Control"); | ||
| response.headers.delete("Cloudflare-CDN-Cache-Control"); | ||
| response.headers.delete("Cache-Tag"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should not need to exist here - vinext shouldnt know about these 3 headers, that's why the cdn cache adapters exist.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 158e082. |
||
| return response; | ||
| } | ||
|
|
||
| const shouldSpeculativelyWriteCache = | ||
| options.isProduction && | ||
| shouldCaptureRscForCacheMetadata && | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean(${globalErrorVar}?.default)correctly treats aglobal-errorfile with no default export as "no custom global error" — so an app with a malformedglobal-error.tsx(file present, no default) would still get the__next_error__recovery shell rather than the server-side boundary path. That's a reasonable choice, but worth confirming it matches Next.js, which I believe validates/throws on aglobal-errorwithout a default export rather than silently falling back. Non-blocking, but a candidate for a follow-up parity check.