-
Notifications
You must be signed in to change notification settings - Fork 292
fix: serve public files in prod #766
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 all commits
34b657d
5a3b769
bfda6ae
ecc58f5
66d3db3
3581158
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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -94,6 +94,8 @@ export type AppRouterConfig = { | |||||||||||||||||
| * `virtual:vinext-server-entry` when this flag is set. | ||||||||||||||||||
| */ | ||||||||||||||||||
| hasPagesDir?: boolean; | ||||||||||||||||||
| /** Exact public/ file routes, using normalized leading-slash pathnames. */ | ||||||||||||||||||
| publicFiles?: string[]; | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
|
|
@@ -123,6 +125,7 @@ export function generateRscEntry( | |||||||||||||||||
| const bodySizeLimit = config?.bodySizeLimit ?? 1 * 1024 * 1024; | ||||||||||||||||||
| const i18nConfig = config?.i18n ?? null; | ||||||||||||||||||
| const hasPagesDir = config?.hasPagesDir ?? false; | ||||||||||||||||||
| const publicFiles = config?.publicFiles ?? []; | ||||||||||||||||||
| // Build import map for all page and layout files | ||||||||||||||||||
| const imports: string[] = []; | ||||||||||||||||||
| const importMap: Map<string, string> = new Map(); | ||||||||||||||||||
|
|
@@ -845,6 +848,21 @@ function matchRoute(url) { | |||||||||||||||||
| return _trieMatch(_routeTrie, urlParts); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| function __createStaticFileSignal(pathname, _mwCtx) { | ||||||||||||||||||
| const headers = new Headers({ | ||||||||||||||||||
| "x-vinext-static-file": encodeURIComponent(pathname), | ||||||||||||||||||
| }); | ||||||||||||||||||
| if (_mwCtx.headers) { | ||||||||||||||||||
| for (const [key, value] of _mwCtx.headers) { | ||||||||||||||||||
| headers.append(key, value); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| return new Response(null, { | ||||||||||||||||||
| status: _mwCtx.status ?? 200, | ||||||||||||||||||
| headers, | ||||||||||||||||||
| }); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // matchPattern is kept for findIntercept (linear scan over small interceptLookup array). | ||||||||||||||||||
| function matchPattern(urlParts, patternParts) { | ||||||||||||||||||
| const params = Object.create(null); | ||||||||||||||||||
|
|
@@ -1214,6 +1232,7 @@ const __i18nConfig = ${JSON.stringify(i18nConfig)}; | |||||||||||||||||
| const __configRedirects = ${JSON.stringify(redirects)}; | ||||||||||||||||||
| const __configRewrites = ${JSON.stringify(rewrites)}; | ||||||||||||||||||
| const __configHeaders = ${JSON.stringify(headers)}; | ||||||||||||||||||
| const __publicFiles = new Set(${JSON.stringify(publicFiles)}); | ||||||||||||||||||
| const __allowedOrigins = ${JSON.stringify(allowedOrigins)}; | ||||||||||||||||||
|
|
||||||||||||||||||
| ${generateDevOriginCheckCode(config?.allowedDevOrigins)} | ||||||||||||||||||
|
|
@@ -1424,6 +1443,9 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||||||||||||||||
| ${ | ||||||||||||||||||
| bp | ||||||||||||||||||
| ? ` | ||||||||||||||||||
| if (!hasBasePath(pathname, __basePath) && !pathname.startsWith("/__vinext/")) { | ||||||||||||||||||
| return new Response("Not Found", { status: 404 }); | ||||||||||||||||||
| } | ||||||||||||||||||
hyoban marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
1443
to
+1448
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 basePath 404 guard is a meaningful behavioral change beyond "serve public files." Previously, requests without the basePath prefix could fall through to catch-all routes or middleware; now they get a hard 404. This matches Next.js behavior and is probably correct, but it should be called out in the PR description since it changes observable behavior for any app with Worth noting: this also blocks middleware from seeing non-basePath requests. In Next.js, middleware's |
||||||||||||||||||
| // Strip basePath prefix | ||||||||||||||||||
| pathname = stripBasePath(pathname, __basePath); | ||||||||||||||||||
| ` | ||||||||||||||||||
|
|
@@ -1781,6 +1803,18 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Serve public/ files as filesystem routes after middleware and before | ||||||||||||||||||
| // afterFiles/fallback rewrites, matching Next.js routing semantics. | ||||||||||||||||||
| if ( | ||||||||||||||||||
| (request.method === "GET" || request.method === "HEAD") && | ||||||||||||||||||
| !pathname.endsWith(".rsc") && | ||||||||||||||||||
|
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. Nit:
Suggested change
|
||||||||||||||||||
| __publicFiles.has(cleanPathname) | ||||||||||||||||||
hyoban marked this conversation as resolved.
Show resolved
Hide resolved
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. Nit: the Also, |
||||||||||||||||||
| ) { | ||||||||||||||||||
| setHeadersContext(null); | ||||||||||||||||||
| setNavigationContext(null); | ||||||||||||||||||
| return __createStaticFileSignal(cleanPathname, _mwCtx); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Set navigation context for Server Components. | ||||||||||||||||||
| // Note: Headers context is already set by runWithRequestContext in the handler wrapper. | ||||||||||||||||||
| setNavigationContext({ | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,9 +15,20 @@ | |
| // @ts-expect-error — virtual module resolved by vinext | ||
| import rscHandler from "virtual:vinext-rsc-entry"; | ||
| import { runWithExecutionContext, type ExecutionContextLike } from "../shims/request-context.js"; | ||
| import { resolveStaticAssetSignal } from "./worker-utils.js"; | ||
|
|
||
| type WorkerAssetEnv = { | ||
| ASSETS?: { | ||
| fetch(request: Request): Promise<Response> | Response; | ||
| }; | ||
| }; | ||
|
|
||
| export default { | ||
| async fetch(request: Request, _env?: unknown, ctx?: ExecutionContextLike): Promise<Response> { | ||
| async fetch( | ||
| request: Request, | ||
| env?: WorkerAssetEnv, | ||
| ctx?: ExecutionContextLike, | ||
| ): Promise<Response> { | ||
| const url = new URL(request.url); | ||
|
|
||
| // Normalize backslashes (browsers treat /\ as //) before any other checks. | ||
|
|
@@ -52,6 +63,13 @@ export default { | |
| const result = await (ctx ? runWithExecutionContext(ctx, handleFn) : handleFn()); | ||
|
|
||
| if (result instanceof Response) { | ||
| if (env?.ASSETS) { | ||
|
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. Clean approach — |
||
| const assetResponse = await resolveStaticAssetSignal(result, { | ||
| fetchAsset: (path) => | ||
| Promise.resolve(env.ASSETS!.fetch(new Request(new URL(path, request.url)))), | ||
| }); | ||
| if (assetResponse) return assetResponse; | ||
|
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. Bug: double signal resolution with deploy-generated workers. The deploy-generated worker entry ( The second call is a no-op (header is stripped), but it still:
More importantly, it's confusing — two layers both claim responsibility for the same signal. Consider gating this on a condition that indicates no outer wrapper will handle it, or remove the signal handling from one of the two layers. The simplest fix: since Alternatively, just remove the signal resolution from |
||
| } | ||
| return result; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -391,8 +391,11 @@ async function tryServeStatic( | |
| compress: boolean, | ||
| cache?: StaticFileCache, | ||
| extraHeaders?: Record<string, string | string[]>, | ||
| statusCode?: number, | ||
| ): Promise<boolean> { | ||
| if (pathname === "/") return false; | ||
| const responseStatus = statusCode ?? 200; | ||
| const omitBody = isNoBodyResponseStatus(responseStatus); | ||
|
|
||
| // ── Fast path: pre-computed headers, minimal per-request work ── | ||
| // When a cache is provided, all path validation happened at startup. | ||
|
|
@@ -419,7 +422,11 @@ async function tryServeStatic( | |
|
|
||
| // 304 Not Modified: string compare against pre-computed ETag | ||
| const ifNoneMatch = req.headers["if-none-match"]; | ||
| if (typeof ifNoneMatch === "string" && matchesIfNoneMatchHeader(ifNoneMatch, entry.etag)) { | ||
| if ( | ||
| responseStatus === 200 && | ||
| typeof ifNoneMatch === "string" && | ||
| matchesIfNoneMatchHeader(ifNoneMatch, entry.etag) | ||
| ) { | ||
| if (extraHeaders) { | ||
| res.writeHead(304, { ...entry.notModifiedHeaders, ...extraHeaders }); | ||
| } else { | ||
|
|
@@ -449,12 +456,12 @@ async function tryServeStatic( | |
| : entry.original; | ||
|
|
||
| if (extraHeaders) { | ||
| res.writeHead(200, { ...variant.headers, ...extraHeaders }); | ||
| res.writeHead(responseStatus, { ...variant.headers, ...extraHeaders }); | ||
| } else { | ||
| res.writeHead(200, variant.headers); | ||
| res.writeHead(responseStatus, variant.headers); | ||
| } | ||
|
|
||
| if (req.method === "HEAD") { | ||
| if (omitBody || req.method === "HEAD") { | ||
| res.end(); | ||
| return true; | ||
| } | ||
|
|
@@ -515,7 +522,11 @@ async function tryServeStatic( | |
| // compress=false also skips all compressed variants. | ||
| // Spreading undefined is a no-op in object literals (ES2018+). | ||
| const ifNoneMatch = req.headers["if-none-match"]; | ||
| if (typeof ifNoneMatch === "string" && matchesIfNoneMatchHeader(ifNoneMatch, etag)) { | ||
| if ( | ||
| responseStatus === 200 && | ||
| typeof ifNoneMatch === "string" && | ||
| matchesIfNoneMatchHeader(ifNoneMatch, etag) | ||
| ) { | ||
| const notModifiedHeaders: Record<string, string | string[]> = { | ||
| ETag: etag, | ||
| "Cache-Control": cacheControl, | ||
|
|
@@ -539,12 +550,12 @@ async function tryServeStatic( | |
| if (encoding) { | ||
| // Content-Length omitted intentionally: compressed size isn't known | ||
| // ahead of time, so Node.js uses chunked transfer encoding. | ||
| res.writeHead(200, { | ||
| res.writeHead(responseStatus, { | ||
| ...baseHeaders, | ||
| "Content-Encoding": encoding, | ||
| Vary: "Accept-Encoding", | ||
| }); | ||
| if (req.method === "HEAD") { | ||
| if (omitBody || req.method === "HEAD") { | ||
| res.end(); | ||
| return true; | ||
| } | ||
|
|
@@ -561,11 +572,11 @@ async function tryServeStatic( | |
| } | ||
| } | ||
|
|
||
| res.writeHead(200, { | ||
| res.writeHead(responseStatus, { | ||
| ...baseHeaders, | ||
| "Content-Length": String(resolved.size), | ||
| }); | ||
| if (req.method === "HEAD") { | ||
| if (omitBody || req.method === "HEAD") { | ||
| res.end(); | ||
| return true; | ||
| } | ||
|
|
@@ -1044,6 +1055,46 @@ async function startAppRouterServer(options: AppRouterServerOptions) { | |
| const request = nodeToWebRequest(req, normalizedUrl); | ||
| const response = await rscHandler(request); | ||
|
|
||
| const staticFileSignal = response.headers.get("x-vinext-static-file"); | ||
hyoban marked this conversation as resolved.
Show resolved
Hide resolved
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 Not blocking — the current test coverage (deploy.test.ts, app-router.test.ts, serve-static.test.ts) would catch a mismatch. |
||
| if (staticFileSignal) { | ||
| let staticFilePath = "/"; | ||
| try { | ||
| staticFilePath = decodeURIComponent(staticFileSignal); | ||
| } catch { | ||
| staticFilePath = staticFileSignal; | ||
| } | ||
|
|
||
| const staticResponseHeaders = omitHeadersCaseInsensitive( | ||
| mergeResponseHeaders({}, response), | ||
| ["x-vinext-static-file", "content-encoding", "content-length", "content-type"], | ||
| ); | ||
|
|
||
| const served = await tryServeStatic( | ||
| req, | ||
| res, | ||
| clientDir, | ||
| staticFilePath, | ||
hyoban marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| compress, | ||
| staticCache, | ||
| staticResponseHeaders, | ||
| response.status, | ||
| ); | ||
| cancelResponseBody(response); | ||
|
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. Good: One edge case: if
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. Minor: |
||
| if (served) { | ||
| return; | ||
| } | ||
| await sendWebResponse( | ||
| new Response("Not Found", { | ||
| status: 404, | ||
| headers: toWebHeaders(staticResponseHeaders), | ||
| }), | ||
| req, | ||
| res, | ||
| compress, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // Stream the Web Response back to the Node.js response | ||
| await sendWebResponse(response, req, res, compress); | ||
| } catch (e) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -35,6 +35,21 @@ function cancelResponseBody(response: Response): void { | |||||||||||
| }); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| function buildHeaderRecord( | ||||||||||||
| response: Response, | ||||||||||||
| omitNames: readonly string[] = [], | ||||||||||||
| ): Record<string, string | string[]> { | ||||||||||||
| const omitted = new Set(omitNames.map((name) => name.toLowerCase())); | ||||||||||||
| const headers: Record<string, string | string[]> = {}; | ||||||||||||
| response.headers.forEach((value, key) => { | ||||||||||||
| if (omitted.has(key.toLowerCase()) || key === "set-cookie") return; | ||||||||||||
| headers[key] = value; | ||||||||||||
| }); | ||||||||||||
| const cookies = response.headers.getSetCookie?.() ?? []; | ||||||||||||
| if (cookies.length > 0) headers["set-cookie"] = cookies; | ||||||||||||
| return headers; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| export function mergeHeaders( | ||||||||||||
| response: Response, | ||||||||||||
| extraHeaders: Record<string, string | string[]>, | ||||||||||||
|
|
@@ -93,3 +108,36 @@ export function mergeHeaders( | |||||||||||
| headers: merged, | ||||||||||||
| }); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| export async function resolveStaticAssetSignal( | ||||||||||||
| signalResponse: Response, | ||||||||||||
| options: { | ||||||||||||
| fetchAsset(path: string): Promise<Response>; | ||||||||||||
| }, | ||||||||||||
| ): Promise<Response | null> { | ||||||||||||
| const signal = signalResponse.headers.get("x-vinext-static-file"); | ||||||||||||
| if (!signal) return null; | ||||||||||||
|
|
||||||||||||
| let assetPath = "/"; | ||||||||||||
| try { | ||||||||||||
| assetPath = decodeURIComponent(signal); | ||||||||||||
| } catch { | ||||||||||||
| assetPath = signal; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const extraHeaders = buildHeaderRecord(signalResponse, [ | ||||||||||||
| "x-vinext-static-file", | ||||||||||||
| "content-encoding", | ||||||||||||
| "content-length", | ||||||||||||
| "content-type", | ||||||||||||
| ]); | ||||||||||||
|
|
||||||||||||
| cancelResponseBody(signalResponse); | ||||||||||||
| const assetResponse = await options.fetchAsset(assetPath); | ||||||||||||
| // Only preserve the middleware/status-layer override when we actually got a | ||||||||||||
| // real asset response back. If the asset lookup misses (404/other non-ok), | ||||||||||||
| // keep that filesystem result instead of masking it with the signal status. | ||||||||||||
| const statusOverride = | ||||||||||||
| assetResponse.ok && signalResponse.status !== 200 ? signalResponse.status : undefined; | ||||||||||||
|
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 intent of this condition could use a comment. When This seems correct (don't override a real 404 with a middleware 403), but it's non-obvious. A one-liner would help:
Suggested change
|
||||||||||||
| return mergeHeaders(assetResponse, extraHeaders, statusOverride); | ||||||||||||
| } | ||||||||||||
Uh oh!
There was an error while loading. Please reload this page.