-
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
base: main
Are you sure you want to change the base?
Changes from all commits
2a9a102
99789d7
dab9c72
7f8b8ba
f8ff07c
ad6daac
2a6292a
57b01b4
2da1242
e9f0119
a2f481b
c760b2a
001f36a
8a259f4
fe3df57
e58a313
98b93df
53aef71
384c40e
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 |
|---|---|---|
|
|
@@ -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"); | ||
| 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.