fix(pages): align middleware data prefetch caching - #2451
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(pages): align middleware data prefetch caching
Reviewed against Next.js source. The core behavior matches Next.js exactly, and test coverage is strong. Verified:
- The middleware-prefetch-skip response (
x-matched-path: <pathname>,x-middleware-skip: 1,cache-control: private, no-cache, no-store, max-age=0, must-revalidate, body{}) matchesbase-server.tsrenderToResponseWithComponentsImpl(the!isSSG && x-middleware-prefetch && !is404Page && pathname !== '/_error'branch) byte-for-byte, including the correctx-matched-pathheader (notx-nextjs-matched-path). - Preserving
x-middleware-cachethrough response shaping is correct: Next.js'sfilterInternalHeaders(server/lib/server-ipc/utils.ts) stripsx-middleware-rewrite/redirect/set-cookie/skip/override-headers/nextbut intentionally does not stripx-middleware-cache, so the client can read the prefetch cache opt-out. _error/_app/_documentare excluded from the Pages route table (routing/pages-router.ts:25), so the skip branch cannot fire for error pages — matching Next.js's!is404Page && pathname !== '/_error'guard.
vp check and the targeted suites (pages-data-prefetch, pages-request-pipeline, pages-data-route, shims, link-navigation, middleware-runtime, request-pipeline, app-post-middleware-context, middleware-runtime-trailing-slash, pages-api-with-middleware) all pass.
Non-blocking observations
1. Reuse the existing MIDDLEWARE_SKIP_HEADER constant. "x-middleware-skip" is hardcoded as a string literal in three new places:
server/pages-data-route.ts:158(response build)server/pages-request-pipeline.ts(via the skip-response builder)shims/internal/pages-data-fetch-dedup.ts:97(client read)
There's already a MIDDLEWARE_SKIP_HEADER = "x-middleware-skip" in server/headers.ts:153 (currently unexported/module-private). Consider exporting it (or moving it next to MIDDLEWARE_CACHE_HEADER in utils/protocol-headers.ts) and referencing it, to keep the protocol header names in one place like the other x-middleware-* constants this PR touches.
2. Dev dataKind computation reads every page file on every request. In index.ts, devPageRouteDataKinds runs readFile + hasExportedName for all page routes on each request (Promise.all over devPageRoutes), even though only the matched route needs classification. devPageRoutes is already recomputed per-request, so this follows the existing pattern, but for apps with many pages it adds N file reads per request in dev. Consider computing dataKind lazily for just the matched route inside matchPageRoute (memoized per request), or reusing whatever the pages-server-entry codegen already knows. Dev-only, so not a blocker.
3. Potential dev/prod dataKind parity gap. Prod (pages-server-entry.ts) derives dataKind from the actual module exports (typeof page_i.getStaticProps === "function"), while dev uses AST static analysis (hasExportedName). These can diverge for pages that re-export data functions (export { getStaticProps } from "./shared") or use export * from. hasExportedName handles named specifiers but not export * / dynamic patterns. Edge case, but worth a comment or a follow-up if such re-export patterns are in scope.
4. readFile rejection surfaces to the whole request in dev. If a page file is deleted mid-request (e.g., during an edit), the Promise.all(...readFile...) rejects and fails the request rather than falling back to "none". A per-file .catch(() => "none") would make the classification resilient, consistent with the ?? "none" fallback used at the lookup site.
None of these block merge — the observable behavior is correct and matches Next.js.
|
Review posted to PR #2451. SummaryI reviewed PR #2451 ( Verified correct
Non-blocking observations raised
None block merge — the observable behavior is correct and Next.js-aligned. |
5edfbb8 to
417efdb
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): align middleware data prefetch caching
Reviewed against the Next.js source in node_modules/next/dist/server/base-server.js and ran the affected suites. The core behavior is correct and matches Next.js. The one item worth confirming is the SSG classification, which I verified against Next.js source below.
Verified correct
1. dataKind !== "static" correctly mirrors Next.js !isSSG. This was the main thing I wanted to confirm, since it's the crux of the change. In base-server.js:
let isSSG = !!components.getStaticProps; // line 1193
// ...
isSSG ||= !!prerenderManifest.routes[toRoute(pathname)]; // line 1249 (prod only)
// ...
if (!isSSG && req.headers['x-middleware-prefetch'] && !(is404Page || pathname === '/_error')) {
res.setHeader(MATCHED_PATH_HEADER, pathname); // x-matched-path
res.setHeader('x-middleware-skip', '1');
res.setHeader('cache-control', 'private, no-cache, no-store, max-age=0, must-revalidate');
res.body('{}').send();
return null;
}So Next.js emits the skip protocol for both getServerSideProps pages and plain pages with no data method (both have getStaticProps === undefined and aren't in prerenderManifest.routes). vinext's condition match.route.dataKind === "static" gates only on getStaticProps, so "server" and "none" both fall through to the skip — matching Next.js. The response shape (x-matched-path: <pathname>, x-middleware-skip: 1, that exact cache-control, body {}) is byte-for-byte identical, including using x-matched-path (not x-nextjs-matched-path).
2. Skip header survives response shaping. buildMiddlewarePrefetchSkipResponse is wrapped in mergeHeaders(skipResponse, middlewareHeaders, ...). mergeHeaders (worker-utils.ts:78-81) applies the response's own headers last, so x-middleware-skip: 1 / x-matched-path win over any staged x-middleware-*. middlewareHeaders were already run through processMiddlewareHeaders, and the pipeline result isn't re-stripped downstream. tests/pages-request-pipeline.test.ts asserts the header survives.
3. x-middleware-cache preservation is correct. The three edits (config-matchers.ts, request-pipeline.ts, middleware-request-headers.ts) that keep x-middleware-cache while still stripping other x-middleware-* match Next.js filterInternalHeaders, which intentionally does not strip it so the client prefetch cache can read the opt-out. fetchCachedPagesData correctly evicts on both x-middleware-cache: no-cache and the new x-middleware-skip sentinel.
4. Tests + vp check pass. Ran pages-data-prefetch, pages-request-pipeline, pages-data-route, shims, link-navigation, middleware-runtime, request-pipeline, entry-templates, routing, route-sorting — all green (1,600+ tests). vp check clean on all changed source files.
Non-blocking observations
1. Missing is404Page / is500Page guard for full parity. Next.js guards with !(is404Page || pathname === '/_error'). _app/_document/_error are excluded from the route table so they can't match, but a custom pages/404.tsx / pages/500.tsx (with no data method) is in the route table (404/500 aren't in RESERVED_PAGE_NAMES). A _next/data/<id>/404.json prefetch with x-middleware-prefetch: 1 would produce the skip response in vinext where Next.js suppresses it via is404Page. In practice the client never prefetches 404/500 data URLs, so this is a narrow parity gap, not a functional bug — but a dataKind/pattern guard for /404 and /500 would close it and is worth a follow-up or a comment.
2. Reuse the existing MIDDLEWARE_SKIP_HEADER constant. "x-middleware-skip" is now hardcoded in pages-data-route.ts:158 and pages-data-fetch-dedup.ts:97, while server/headers.ts:153 already defines MIDDLEWARE_SKIP_HEADER (currently module-private). This PR just added MIDDLEWARE_CACHE_HEADER to utils/protocol-headers.ts — colocating/exporting the skip constant there and referencing it keeps the protocol header names in one place.
3. Dev dataKind reads every page file per request. index.ts devPageRouteDataKinds runs readFile + hasExportedName over all devPageRoutes on every dev request, though only the matched route needs it. Dev-only, follows the existing per-request devPageRoutes pattern, but scales O(N pages) per request. Consider classifying lazily for just the matched route (memoized per request). Also, the Promise.all(readFile...) has no per-file .catch — a file deleted mid-edit rejects the whole request instead of falling back to "none"; a .catch(() => "none") per read would be more resilient and consistent with the ?? "none" at the lookup site.
4. Re-export dataKind classification is name-sensitive (pre-existing). hasExportedName matches on specifier.local, so export { getStaticProps as loader } is treated as dataKind: "static" even though the exported name isn't getStaticProps, while export { loader as getStaticProps } is not. The client SSG manifest path classifies these the opposite way (see entry-templates.test.ts:1249), so dev/prod can disagree for renamed re-exports. This mirrors the existing manifest behavior and isn't introduced here, but the two classifiers diverging is worth a follow-up.
None of these block merge — the observable behavior matches Next.js.
|
Review posted to PR #2451. SummaryI reviewed PR #2451 ( Key verificationThe crux of the PR is the Non-blocking observations raised
None block merge — observable behavior matches Next.js. |
NathanDrake2406
left a comment
NathanDrake2406
left a comment
There was a problem hiding this comment.
Requesting changes on the shape of the new dataKind contract. The protocol behaviour looks reasonable, but the implementation makes the route classification both scattered and non-conservative at the pipeline boundary. This is not worth carrying as another optional string through the request flow.
|
|
||
| async function getPagesDataKind(filePath: string): Promise<"static" | "server" | "none"> { | ||
| const source = await readFile(filePath, "utf8"); | ||
| if (hasExportedName(source, "getStaticProps")) return "static"; |
There was a problem hiding this comment.
This feels like the point where dataKind needs a single owner rather than another local source parser. The client entry already classifies Pages routes for __VINEXT_PAGES_SSG_PATTERNS__ / __VINEXT_PAGES_SSP_PATTERNS__, this adds a second build-time classifier, and dev adds a third request-time classifier in index.ts. That leaves one protocol decision split across three file-read/AST paths. Can we pull this into a shared collectPagesDataKinds(pageRoutes) (or enrich the route metadata once) and have client entry, server entry, and dev consume the same map? That would delete the duplicated parsing and let the pipeline require a real dataKind instead of accepting an optional one.
| type PageRouteMatch = { | ||
| route: { isDynamic: boolean; pattern?: string; dataKind?: "static" | "server" | "none" }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
dataKind shouldn't be optional at this boundary while the skip branch treats every non-"static" value as eligible for the prefetch-skip response. If classification is absent/unknown, we now take the unsafe path and bypass rendering; pre-PR behaviour would have rendered. Please either make dataKind required for every matchPageRoute result, or make the skip check explicit (dataKind === "server" || dataKind === "none") so undefined preserves the old render path.
|
Worker lane coverage note for deploy-suite run 28478866791 / job 84413308650: this PR covers the middleware-rewrites prefetch failures in this assignment ( |
|
Backlog mapping for GitHub Actions run 28478866791 / job 84413308650: this PR appears to own the Pages Router middleware data-prefetch caching failures, alongside the broader middleware rewrite/navigation-state coverage in #2454. Per instruction I am skipping a duplicate implementation because this PR is by |
…tch-cache # Conflicts: # packages/vinext/src/shims/link.tsx
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
…tch-cache # Conflicts: # packages/vinext/src/shims/router.ts
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
…tch-cache # Conflicts: # packages/vinext/src/server/pages-request-pipeline.ts
There was a problem hiding this comment.
Review: fix(pages): align middleware data prefetch caching
Reviewed the source changes against the Next.js implementation in node_modules/next/dist/server/base-server.js and node_modules/next/dist/server/lib/server-ipc/utils.js, and ran the affected suites (pages-request-pipeline, pages-data-route, pages-data-prefetch, shims, link-navigation, middleware-runtime, request-pipeline, entry-templates — all green; vp check clean on the changed source files). The observable behavior is correct and matches Next.js production. No blocking issues.
Verified correct
1. Skip response shape matches base-server.js:1256-1262 byte-for-byte. buildMiddlewarePrefetchSkipResponse emits x-matched-path: <pathname>, x-middleware-skip: 1, cache-control: private, no-cache, no-store, max-age=0, must-revalidate, body {}, using the correct x-matched-path (not x-nextjs-matched-path).
2. The dataKind-based gate matches Next.js prod semantics — including the "none" exclusion that earlier reviews questioned. The condition only fires for dataKind === "server", so plain no-data pages ("none") and getStaticProps pages ("static") both fall through to render. This is easy to misread against base-server.js:1193 (isSSG = !!getStaticProps) alone, but line 1249 augments it in production: isSSG ||= !!prerenderManifest.routes[toRoute(pathname)]. A plain automatically-static-optimized page is in prerenderManifest.routes, so isSSG becomes true and Next.js does not skip it. So excluding "none" is the correct prod behavior, and matching NathanDrake2406's earlier suggestion to also skip "none" would have introduced a prod divergence.
The only residual difference is dev-only: isSSG is not manifest-augmented in dev (else if (!this.dev) at line 1248), so Next.js dev would skip a "none" page while vinext renders it. That is a harmless, safe divergence (render vs. empty {}), and dev/prod parity for the dataKind value itself is preserved because both the prod route table (entries/pages-server-entry.ts) and the dev matcher (index.ts classifyDevPageRoute) classify via hasExportedName.
3. undefined dataKind takes the safe render path. (dataKind !== "static" && dataKind !== "server") returns null (no skip) for undefined, so an unknown/absent classification preserves pre-PR render behavior. This addresses the conservative-boundary concern from the earlier requested-changes review.
4. x-middleware-cache preservation matches Next.js filterInternalHeaders. The INTERNAL_HEADERS list in server-ipc/utils.js:62-74 strips x-middleware-rewrite/redirect/set-cookie/skip/override-headers/next but intentionally does not include x-middleware-cache, so the client prefetch cache can read the opt-out. The three edits (config-matchers.ts:594, request-pipeline.ts:556, middleware-request-headers.ts shouldKeepMiddlewareHeader) correctly special-case it while still stripping the rest.
5. External-proxy stripping is correctly left untouched. config-matchers.ts:1377 (proxyExternalRequest) still strips all x-middleware-* including cache before forwarding to external destinations — correct, since internal signals must not leak to third parties.
6. Skip header survives shaping. The skip response is mergeHeaders(skipResponse, middlewareHeaders, undefined) with the response's own headers applied last, so x-middleware-skip / x-matched-path win. fetchCachedPagesData correctly evicts on both x-middleware-cache: no-cache and the new x-middleware-skip sentinel.
Non-blocking observations
1. The skip gate is more convoluted than it needs to be. In pages-request-pipeline.ts:442-448:
if (
(dataKind !== "static" && dataKind !== "server") ||
!isDataRequest ||
!deps.hasMiddleware ||
dataKind === "static" ||
request.headers.get("x-middleware-prefetch") !== "1"
)The first clause and the dataKind === "static" clause together reduce to just dataKind !== "server". Consider simplifying to dataKind !== "server" || !isDataRequest || !deps.hasMiddleware || prefetch !== "1" for readability — the current form reads as if "static" is handled two different ways.
2. Hardcoded "x-middleware-skip" literal despite an existing constant. pages-data-route.ts:163, pages-data-fetch-dedup.ts:97, and the pipeline all hardcode the string, while server/headers.ts:158 already defines MIDDLEWARE_SKIP_HEADER (module-private). This PR just added MIDDLEWARE_CACHE_HEADER to utils/protocol-headers.ts — colocating/exporting the skip constant there and referencing it would keep all x-middleware-* protocol names in one place, consistent with the rest of this change.
3. fetchUncachedPagesData fully buffers a body it discards. For non-SSG middleware prefetches it does arrayBuffer() then throws the buffer away. The intent (warm middleware without persisting to the static data cache) is correct and matches the PR goal, but the full buffering is slightly wasteful; a void response.body?.cancel() or reading nothing would avoid materializing the payload. Minor.
None of these block merge — the behavior matches Next.js production.
|
Review posted to PR #2451. SummaryI reviewed PR #2451 ( Key verificationThe crux of the PR — the
Both were based on an incomplete read of Next.js. Line 1249 augments Also verified: skip response shape is byte-for-byte identical to Non-blocking observations raised
The latest commits already addressed the earlier dev-perf and error-resilience observations (lazy per-route classification with memoization + try/catch fallback). |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): align middleware data prefetch caching
Reviewed the current head (01a692b, which changed the skip gate from !== "static" to === "server" since the earlier reviews) against the Next.js implementation in node_modules/next/dist/server/base-server.js + shared/lib/router/router.js, and ran the affected suites (pages-request-pipeline, pages-data-prefetch, pages-data-route, shims, link-navigation, middleware-runtime, request-pipeline, entry-templates — all green; tsc --noEmit clean). The observable behavior matches Next.js production. No blocking issues.
Verified correct
1. The dataKind === "server" gate is the correct prod semantics. base-server.js:1256 fires the skip on !isSSG. In production isSSG is !!getStaticProps (1193) augmented by isSSG ||= !!prerenderManifest.routes[toRoute(pathname)] (1249, prod-only). So a plain automatically-static-optimized ("none") page is in the prerender manifest → isSSG = true → not skipped; only getServerSideProps ("server") pages skip. The latest commit gating on dataKind === "server" matches this exactly. undefined falls through to render (safe). This resolves the earlier "none"-inclusion debate — including "none" would have been a prod divergence.
2. Skip response shape matches base-server.js:1257-1260 byte-for-byte — x-matched-path: <pathname>, x-middleware-skip: 1, cache-control: private, no-cache, no-store, max-age=0, must-revalidate, body {}. For dynamic routes, Next.js sets x-matched-path to match.definition.pathname (the route pattern, e.g. /blog/[slug], per line 1566), and vinext's matchedPathnameForRoute → patternToNextFormat(pattern) produces the same [slug] form. Correct.
3. Client-side skip handling matches router.js:1475-1481. Next.js reads x-middleware-skip off the cached prefetch response and does delete this.sdc[data.dataHref] to force a full refetch on navigation. vinext's fetchCachedPagesData evicting on MIDDLEWARE_SKIP_HEADER !== null mirrors this. Note the skip response only reaches the cache path for SSG prefetches (GSSP prefetches now go through the non-caching fetchUncachedPagesData), which is consistent with Next.js only ever bailing non-SSG prefetches.
4. x-middleware-cache preservation matches Next.js filterInternalHeaders. The three edits (config-matchers.ts, request-pipeline.ts, middleware-request-headers.ts) special-case it while still stripping the rest of x-middleware-*; INTERNAL_HEADERS in server-ipc/utils.js deliberately omits it. External-proxy stripping (config-matchers.ts proxyExternalRequest) is correctly left stripping everything.
5. dataKind classifier is consistent across build/dev. Prod (pages-server-entry.ts getPagesDataKind) and dev (index.ts classifyDevPageRoute) both use the same hasExportedName AST classifier from build/report.js, the same one the client SSG/SSP manifest uses. No new divergence introduced. The prod route table now embeds dataKind statically and matchPageRoute/matchRoute forwards it; the dev adapter builds { route: { dataKind, isDynamic, pattern } }. Wiring is complete on both paths.
6. Dev classifier addresses the earlier perf/resilience notes. devPageRouteDataKinds is created per request and only the matched route is classified lazily (not all routes), with a try/catch fallback to "none" for mid-edit file races. Per-request scope also means no stale-cache-after-edit problem. Good.
Non-blocking observations
1. Dev-only parity gap for "none" pages (acceptable). Next.js dev skips manifest augmentation (else if (!this.dev) at 1248), so Next.js dev would emit the skip for a plain "none" page while vinext dev renders it. Harmless (render vs empty {}), and prod parity is exact. Worth a one-line comment near the gate so a future reader doesn't "fix" it back to !== "static".
2. classifyDevPageRoute uses fs.readFileSync (sync/blocking) on the dev request path. Dev-only, once per matched route per request, and Vite dev already does sync I/O, so acceptable — but an async read would be tidier if this ever moves off the hot path.
3. fetchUncachedPagesData buffers a body it discards and drops init.signal. For GSSP middleware prefetches it does arrayBuffer() then rebuilds a Response that the caller ignores. void response.body?.cancel() (or just not reading) avoids materializing the payload, and forwarding the signal would let aborted prefetches cancel in-flight. Fire-and-forget best-effort, so not a leak, but slightly wasteful.
4. No test for the i18n branch of matchedPathnameForRoute on the skip response. The locale-prefixing branch (lines 428-433) is uncovered by the skip-response tests. Since vinext's client keys off x-middleware-skip presence (not x-matched-path), the exact value is largely cosmetic, but a test would lock in the i18n shape.
None of these block merge — the behavior matches Next.js production and all targeted suites pass.
|
Review posted to PR #2451. SummaryI reviewed PR #2451 ( Key verificationThe important detail is that the latest commit changed the skip gate from
Also verified: skip response shape is byte-for-byte identical (including the dynamic-route Non-blocking observations raised
The dev-perf/resilience concerns from earlier reviews are already addressed (per-request lazy classification of only the matched route + try/catch fallback). |
2 participants
Summary
x-middleware-cachethrough middleware response shaping so SSG middleware prefetches can opt out of the client cacheValidation
vp check.test/e2e/middleware-rewrites/test/index.test.tsstill has an unrelated broader timeout inshould handle static rewrite from next.config.js correctly, and a later direct Next harness attempt for one assertion failed before assertions due to the local.nextjs-refSWC setup mismatch.