Repository navigation
fix(pages): answer middleware data misses with a 404 - #3912
james-elicx wants to merge 4 commits into
Conversation
Next.js router-server.ts answers a `_next/data` request that matches no
page with a 404. In a Pages-only app with middleware it is a JSON `{}`
naming the requested path in `x-nextjs-matched-path`; in a hybrid app it
goes to the App not-found route, without the matched path. vinext answered
every such miss with `200 {}` whenever the app had middleware.
Pages misses (prod, Worker and dev) now return the JSON 404 with the
matched path, and hybrid misses return a 404 without it. A hybrid data
request that middleware rewrites onto an App route keeps its 200
placeholder, which carries `x-nextjs-rewrite` for the client's hard
navigation.
|
/bigbonk review for issues |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| // An App match is served (the Pages client hard-navigates to it). A miss | ||
| // goes to the App not-found route in Next.js router-server.ts, so unlike | ||
| // a Pages-only app it gets a 404 without a matched path. | ||
| if (!match) return new Response("{}", { status: 404, headers }); |
There was a problem hiding this comment.
The new hybrid miss bypasses withUnmatchedRouteCacheControl, so after finalizeAppRscResponse a matching headers() rule (or the origin-managed adapter default) can leave this 404 with a cacheable policy or no policy at all. Next.js sets Cache-Control: private, no-cache, no-store, max-age=0, must-revalidate in router-server.ts before taking this data-miss shortcut. The same omission exists in the new 404 exits in pages-request-pipeline.ts and dev-server.ts; please apply/reassert the unmatched-route cache policy in all three paths and cover it in the updated tests.
There was a problem hiding this comment.
Fixed in 46ed950: the hybrid misses (with and without middleware) go through withUnmatchedRouteCacheControl, the Pages pipeline exit applies NEVER_CACHE_CONTROL on the response headers (these take precedence over staged config headers in mergeHeaders), and the dev data-miss exit sends it too. The unit tests cover a matching next.config Cache-Control rule; the integration and e2e tests assert the header.
|
Review: 1 findings. Posted one actionable inline warning. No branch modifications were made. |
Performance benchmarksCompared 0 improved · 0 regressed · 7 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
|
/bigbonk review for issues Please review exact head |
| const notFoundResponse = new Response("{}", { status: 200, headers }); | ||
| // Response headers win over the staged config headers in mergeHeaders, | ||
| // so a next.config Cache-Control rule can't replace this policy. | ||
| applyCdnResponseHeaders(headers, { cacheControl: NEVER_CACHE_CONTROL }); |
There was a problem hiding this comment.
Apply the adapter policy after the final header merge. applyCdnResponseHeaders removes adapter-owned headers only from this temporary headers object, but mergeHeaders then restores any staged CDN-Cache-Control or Cloudflare-CDN-Cache-Control value from matchedPathHeaders. With CloudflareCdnCacheAdapter, those headers take precedence over the private Cache-Control, so a matching config rule or continuing middleware can still make this 404 edge-cacheable. Merge first, then call applyCdnResponseHeaders on the merged response headers (and add an adapter-backed regression test).
There was a problem hiding this comment.
Fixed in cc2e7e5: the pipeline now merges the staged headers first and calls applyCdnResponseHeaders on the merged response's headers. Added a CloudflareCdnCacheAdapter-backed test where a config rule stages Cloudflare-CDN-Cache-Control and middleware stages CDN-Cache-Control; both are dropped, and it fails on the previous head.
|
Since last review: 1 resolved, 0 still open, 1 new. Posted one actionable inline warning. No branch modifications were made. |
|
/bigbonk review for issues Please review exact head |
| ); | ||
| // Applied after the merge, so staged config or middleware headers can't | ||
| // restore a cacheable policy (including adapter-owned edge headers). | ||
| applyCdnResponseHeaders(notFoundResponse.headers, { cacheControl: NEVER_CACHE_CONTROL }); |
There was a problem hiding this comment.
This policy is still overwritten in the multi-stage Pages path. On an unmatched GET, pages-request-stage-entry.ts may dispatch the speculative 404 through the shared response stage; a middleware Cache-Control value is captured in sharedOuterPolicyHeaders, and after this return lines 672-674 call reconcileCdnResponseHeadersAfterOuterPolicy, whose lines 164-165 restore that value verbatim. Consequently middleware returning Cache-Control: public, s-maxage=3600 makes this data-miss 404 cacheable despite this reassertion. Apply the never-cache policy after that reconciliation (or prevent reconciliation for this synthetic response), and cover the request-stage caller rather than only runPagesRequest directly.
There was a problem hiding this comment.
Fixed in b109a06: the pipeline result for this synthetic 404 now carries finalCachePolicy: true, and pages-request-stage-entry.ts skips reconcileCdnResponseHeadersAfterOuterPolicy for it. Added a request-stage test in pages-request-worker-stage.test.ts (middleware Cache-Control: public, s-maxage=3600, a shared speculative 404 dispatch) that fails on the previous head.
|
Review: 1 findings. Posted one actionable inline warning. Targeted tests passed; no branch modifications. |
|
/bigbonk review for issues Please review exact head |
1 participant
Follow-up to #3859 and #3872.
When an app has middleware, vinext answered every
_next/datarequest that matched no page with200 {}. Real Next.js 16.2.7 answers these with a 404, which makes the Pages client hard-navigate:{},x-nextjs-matched-path: /en/nope{}+ matched path{}+ matched path{}+ decoded matched path{}, no matched path{}The table comes from
next build --webpack+next startprobes of a hybrid and a Pages-only app (i18n en/fr, with and without middleware), usingx-nextjs-data: 1requests.The source of this behaviour is Next's
router-server.ts. A miss goes throughinvokeRender('/404'). When the app has middleware matchers, that returnsres.statusCode = 404with{}and the matched path. A hybrid app goes to/_not-foundinstead, so it never takes that branch.The
200 {}that vinext had ported (#2239) comes frombase-server.ts. It only fires when the router matched a page file but the page renderer then couldn't. None of Next's own tests expect 200 for a data miss.middleware-rewriteschecks that the client hard-navigates on a data 404. vinext's client already hard-navigates on these: a miss has no page loader, so it fetches the HTML.Changes
x-nextjs-matched-path. A data request that middleware rewrites onto an App route keeps its 200 placeholder: Next serves the App page there, and the placeholder'sx-nextjs-rewritedrives the client's hard navigation.Cache-Control: private, no-cache, no-store, max-age=0, must-revalidate, which Next'srouter-server.tssets before taking the 404 path. A matching next.configCache-Controlrule can't replace it.Tests
pages-request-pipeline,app-rsc-handler,pages-router,features), which asserted the old 200. They now also assert the never-cache policy.pages-router-prod(pages-basic) andapp-router-encoded-basepath-i18n(app-basic, hybrid). Both fail with 200 without this change.Known limitations
200 {}when the percent-decoded path names a static page (/_next/data/<id>/%61bout.json). That is base-server's branch, after the router matched the decoded file. vinext routes encoded aliases by their raw identity (fix(router): route hybrid Pages fallbacks by the raw request pathname #3859), so it returns the 404.{}body (withx-nextjs-deployment-id, Pages Router: deployment-id header missing on SSG_next/dataroutes #1829). The status matches.