[codex] Add first-pass Pages Router deploy suite parity#922
[codex] Add first-pass Pages Router deploy suite parity#922southpolesteve wants to merge 14 commits intomainfrom
Conversation
commit: |
|
|
/bigbonk review this please |
There was a problem hiding this comment.
First-pass review focused on dev/prod parity, Next.js parity, the new plugins, and the workflow/scripts. Overall this is a substantial and reasonable foundation for Pages Router deploy parity — I did not find any blocking security regressions, the workflow is correctly gated, and the shell scripts are well-written. The biggest concrete parity gap I'd want fixed before this is the canonical Pages Router suite is the data-request redirect format, plus a small number of dev/prod skew items worth tracking.
Blocking concerns
None that I'd consider hard blockers given this is explicitly a first pass.
Parity concerns (Next.js behavior gaps)
-
_next/data/...jsonredirect shape fromgetStaticProps/getServerSideProps(packages/vinext/src/server/pages-page-data.ts:376-384and:525-532): For data requests, vinext returns a plain307/308with aLocationheader. Next.js's data layer expects eitherx-nextjs-redirect: <dest>(modern) or a JSON body with__N_REDIRECTfor the client router to perform the redirect. The middleware path inprod-server.ts:1909already usesx-nextjs-redirectfor data requests; gSSP/gSP need the same treatment. Same gap exists indev-server.ts:538-552. A meaningful fraction of the Next.js Pages Router data-redirect deploy tests will fail until this is fixed. -
getStaticProps/getServerSidePropsnotFoundfor data requests (pages-page-data.ts:386-396,dev-server.ts:553-565): Dev and the data path return JSON 404 /buildPagesDataNotFoundResponsefor data requests, but the dev-server falls through torenderErrorPage(HTML) regardless ofisDataRequest. Pages Router data requests should always return JSON{"notFound": true}(or 404 withx-nextjs-redirectstyle). Worth aligning with the prod path. -
Config headers run after middleware (
prod-server.ts:2087-2116,index.ts:3107-3113): AGENTS.md flags this as a known parity gap — config headers should run before middleware per the Next.js execution order. The PR perpetuates the gap, but at least dev and prod are consistent. The code already evaluates has/missing against the pre-middleware request snapshot, so the only remaining divergence is the timing of when headers are emitted relative to middleware response headers. Worth a tracking issue if there isn't one. -
matchConfigPatterncase-insensitivity widening (config/config-matchers.ts:722, 750-753, 773): The regex compile now uses the"i"flag and the simple-segment matcher does case-insensitive comparisons. This more closely matches Next.js, but it's a behavior change worth a regression note: anyredirects/rewritessource that previously relied on case-sensitive matching for paths (rare but possible) will now match more permissively. Captures still preserve original case from the request, so destinations are unaffected. -
Middleware case for
pattern.includes(":")(config-matchers.ts:668): Addingpattern.includes(":")to the regex-branch trigger funnels every parameterized source through the regex engine instead of the simple-segment matcher. Functionally equivalent for the common case but increases the per-rule cache footprint and rules out a few simple-matcher fast paths. Acceptable, but worth a perf check on apps with hundreds of rules. -
Pages router
javascript:policy is unchanged (shims/router.ts:1513-1520):router.push()/router.replace()still pass throughjavascript:towindow.location.assign(). Per AGENTS.md this matches Next.js intentionally — so this is correct behavior, not a regression. Just calling it out so reviewers don't flag it.
Correctness & edge cases
-
prod-server.tsanddev-server.tsskew in middleware-driven path normalization:prod-server.ts:1853-1857buildsreqCtxbefore middleware runs (so config redirects/headers see the original request), whiledev-server.ts(viaindex.ts:2916-2918) takes a similar pre-middleware snapshot. Both look correct after this PR — good. One nit: in prod,requestRedirects.lengthis computed every request fromrulesForRequestBasePath(...)(allocating a filtered array even when nothing matches). For high-RPS workloads this could be cached at config-load time keyed byrequestHadBasePath. Not blocking. -
mergeWebResponseand streamed-response Content-Length (prod-server.ts:265-312): Correctly preservesSet-Cookiearray-style headers viagetSetCookie()and stripsContent-Lengthfor streamed HTML. The 204/205/304 body-drop path correctly cancels the upstream body stream. Looks good. -
pages-page-response.ts:414setsContent-Type: text/htmlunconditionally insideapplyGsspHeaders. Verified upstream that this helper is only called on the non-data render path; data requests construct their ownapplication/jsonheaders in the entry. OK, but a comment onapplyGsspHeaderssaying "non-data path only" would prevent future misuse. -
config-matchers.ts:722— adding"i"to the compiled redirect/rewrite regex makes captures case-insensitive but the captured value is whatever appears in the request path. Tests/fixtures should cover a redirect like/Foo/:slug->/foo/:slugto verify the casing of the captured:slugis preserved. -
edge-blob-assets.ts:17-23:isExternalSpecifierusesnew URL(specifier)and considers anything with a non-emptyprotocolexternal. This will treat Windows drive-letter paths likeC:\foo.pngas havingprotocol === "c:"and skip them. On Windows that meansimport x from "C:/foo.png"won't be inlined. Probably acceptable since absolute Windows paths aren't typical in source, but worth a comment. -
edge-blob-assets.ts:73usesoutput.replaceAll(fullMatch, ...)which works because the literalnew URL("foo.png", import.meta.url)text is unique enough, but if the same expression appears twice with different surrounding context the replacement is uniform. Fine. Consider switching toMagicStringfor consistency withimport-meta-url.tsand proper sourcemaps. -
import-meta-url.ts: Looks careful — skips strings, comments, andnew URL(..., import.meta.url)bases. ThefindEnclosingParenlook-back is bounded but on minified code with very long lines could regress to O(n²). Probably fine since the file extension filter excludes.min.js. -
wasm-module.ts:50usesnew WebAssembly.Module(bytes)synchronously at module evaluation. On Cloudflare Workers, large WASM modules hit a 50 ms CPU cap on synchronous compilation; considerWebAssembly.compile()(async). Not blocking — the existing comment correctly notes the Cloudflare Vite plugin already handles?modulenatively and this is the fallback only. -
server-externals.ts: Pure Node-side helper, only used at build time inside the Vite plugin. Won't reach the Workers bundle. The default externals (better-sqlite3,sqlite3,typescript) are reasonable. ThefindPackageJsonFromResolvedFilewalk has a correct termination condition (parent === dir). -
Form shim ordering (
shims/form.tsx:363-364):e.preventDefault()is called aftercreateFormSubmitDestinationUrl(). Ifnew URL(action, window.location.href)throws, the form will submit natively. A defensivetry/catchhere would prevent silent failure modes. Minor. -
pages-server-entry.tsmiddleware code generation (:212-231): Builds aNextRequestwith__mwNextConfigto exposenextUrl.basePathandnextUrl.locale. Looks right. Note themwRequest = new Request(mwUrl, request.clone())pattern at line 228 — this clones the body, which is correct for streaming, but the new Request is then wrapped again asNextRequestat 232. That's two wrappers on a hot path; consider building the NextRequest directly with the canonical URL to avoid one allocation per matched request.
Maintainability
-
Generated entry growth (
tests/__snapshots__/entry-templates.test.ts.snap+836 lines): Most growth is reused option-bag passing into__resolvePagesPageData/__renderPagesPageResponserepeated across multiple snapshot fixtures, which is fine and consistent with AGENTS.md guidance. The one piece of substantive new logic added directly toentries/app-rsc-entry.tsis thex-vinext-loading-payload === "1"handler (rendersloading.tsxto an RSC stream and returns it). That's ~25 lines ofrenderToReadableStreamcall, header construction, and a 204 fallback — borderline. Consider extracting it toserver/app-loading-handler.tsso the entry remains a thin wiring layer, especially since this code is duplicated across all four App Router fixture snapshots. -
pages-server-entry.ts(+593 lines) is mostly delegation to__resolvePagesPageData/__renderPagesPageResponse/__createPagesReqRes. Acceptable. The_renderPagefunction's local i18n + query-derivation block (lines 859-967) is cohesive enough to keep inline, but if it grows further aserver/pages-render-context.tshelper would help. -
prod-server.ts(+652 lines): The Pages Router production server handler (startPagesRouterServer, ~825 lines starting at line 1533) is large. The execution-order pipeline (steps 1-11 with comments) is readable, but the function itself would benefit from extracting the per-request handler into a separate module so the listener registration stays light. Not urgent. -
Dev/prod parity gaps to track: dev-server.ts has its own scope-bound implementation of trailing-slash redirects, basePath stripping, and gSSP/gSP redirect handling that doesn't reuse the prod helpers (
normalizeTrailingSlashPathAndSearch,buildPagesDataRedirectLocation,parsePagesDataRequest). Pulling these into shared helpers would prevent future drift.
Nits
prod-server.ts:1015regex/(?:^|\/)pages[-.]/for finding the Pages client entry could match unrelated assets namedxpages-*if they ever existed. Anchor with^or/is already there but the pattern is loose.config-matchers.ts:96_LOCALE_STATIC_REdoesn't tolerate trailing/in the suffix; mostly fine but worth a brief comment.nextjs-deploy-suite-deploy.sh:188setsIMMUTABLE_ASSET_TOKEN="undefined"(the literal string"undefined") and then never uses it. Looks like dead code from porting; remove.nextjs-deploy-suite-deploy.sh:51BUILD_LOG=".vinext-deploy-build.log"paths are relative to the test app cwd.cleanup.shuses the same names — fine, but if the test runner ever changes cwd between phases this would silently fail. Consider passing absolute paths via env.nextjs-deploy-suite-deploy.sh:347, 350print fixed warning strings to the build log to mimic Next.js's data-size warnings. This is a behavioral fake — if Next.js's deploy suite ever asserts on the exact build-log line shape this will work, but the magic string fragility is worth a comment.- Workflow
.github/workflows/nextjs-pages-router-deploy-suite.ymlis correctlyworkflow_dispatch-only withpermissions: contents: readand no secret usage. ✅ scripts/run-nextjs-deploy-suite.shcorrectly usesset -euo pipefailand quotes all variable expansions. ✅- Tests in
tests/router-scroll-restoration.test.tslook non-tautological — they exercise sessionStorage round-trips, scrollRestoration mode, andpopstatehandling against a fake window with proper getter/setter semantics. Theimport("...?scroll-restoration-enabled")query suffix to bust module cache is unusual but works.
Could not verify against Next.js source — local .nextjs-ref/ clone is not present.
|
The review has been posted to PR #922. SummaryI delegated the review of this 12,657-addition / 100-file PR to a thorough reviewer. The key findings posted to PR #922: Blocking-leaning concerns:
Parity / maintainability:
Verified safe:
|
Summary
This is a first-pass Pages Router adapter-parity slice against the Next.js deploy suite. The goal is intentionally narrow: make the non-
app-dir/ Pages Router deploy suites pass and give us an opt-in way to keep checking them without adding a 200+ suite tax to every PR.This is not claiming full Next.js deploy-suite parity. It does not cover the full ~795-suite manifest, and it does not make the new deploy-suite job a required PR check.
What changed
import.meta.url, styled-jsx, SWC helpers, and trace metadata.Next.js Pages Router Deploy Suite, triggered withworkflow_dispatchand sharded 16 ways. This is deliberately on-demand only and should not run on every PR.test/deploy-tests-manifest.json, excludingtest/e2e/app-dir/**so canary drift does not silently expand the scope.Validation
vp checkpassed after rebasing onto latestorigin/main(598782e), with warning-only lint/type output.vp run vinext#buildpassed.207suites and0app-dir suites.207/207suites passing.0/2:module-layer,og-api,getserversideprops, andopentelemetry/client-trace-metadata.Notes
The workflow is intentionally manual and heavily sharded so we can run it when we want broad adapter confidence without making normal PR CI unbearably slow. Follow-up PRs can continue shrinking this by moving the highest-value cases into the fast vinext-native test suite.