[codex] Add first-pass Pages Router deploy suite parity#922
[codex] Add first-pass Pages Router deploy suite parity#922southpolesteve wants to merge 18 commits into
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:
|
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
Add the workflow, scripts, and manifest generator for running the Next.js Pages Router deploy test suite against vinext. Triggers: - Nightly cron (06:00 UTC) against main - Manual dispatch with configurable vinext-ref, next-ref, and concurrency Extracted from #922.
Add the workflow, scripts, and manifest generator for running the Next.js Pages Router deploy test suite against vinext. Triggers: - Nightly cron (06:00 UTC) against main - Manual dispatch with configurable vinext-ref, next-ref, and concurrency Extracted from #922.
* ci: add Next.js Pages Router deploy suite harness Add the workflow, scripts, and manifest generator for running the Next.js Pages Router deploy test suite against vinext. Triggers: - Nightly cron (06:00 UTC) against main - Manual dispatch with configurable vinext-ref, next-ref, and concurrency Extracted from #922. * refactor: align deploy suite harness with Next.js adapter testing docs - Rename scripts to match Next.js convention: e2e-deploy.sh, e2e-logs.sh, e2e-cleanup.sh - Split workflow into build + test jobs (build once, shard 16 ways) per the documented adapter testing pattern - Generalize workflow from pages-only to configurable: add suite-filter input (pages/app/all, defaults to pages) - Add --filter flag to manifest generator (replaces the pages-hardcoded version) - Accept ADAPTER_DIR (Next.js docs convention) alongside VINEXT_DIR - Rename workflow file to nextjs-deploy-suite.yml - Use matrix.group strings ("1/16") instead of shard integers, matching the docs example * ci: default suite-filter to app * ci: bump Next.js default to v16.2.6, move cron to 02:00 UTC * address review: add report job, clean up cache paths, document compat shims - Add a report job that aggregates .results.json across all shards into a GitHub Actions summary (pass/fail/skip counts, failed test details, passed suite list) and uploads a JSON report artifact - Upload test results from every shard (not just on failure) - Remove redundant next.js/ from cache paths (already under .) - Add comment explaining why IS_TURBOPACK_TEST=1 is needed - Clean up stale PID file in e2e-cleanup.sh - Add header comments to all scripts explaining their purpose and contract - Document the hardcoded warning messages and .next/trace shim - Document the YAML catalog parser assumptions - Fix [[ ]] -> [ ] inconsistency in run-nextjs-deploy-suite.sh * ci: add temporary PR trigger to test workflow [remove before merge] * fix: address deploy script failures found in CI and local testing Bugs found and fixed: - IMMUTABLE_ASSET_TOKEN marker renamed to NEXT_SUPPORTS_IMMUTABLE_ASSETS to match what next-deploy.ts parseIdsFromCliOuput() actually parses. Without this, every test fails with 'Failed to get supportsImmutableAssets'. - Deploy script now injects "type": "module" into test app package.json and generates a vite.config.ts. Without type:module, Rolldown emits .mjs files but the RSC plugin's cross-environment imports expect .js. This mirrors what `vinext init` does (steps 3 and 5). - Added vp (Vite+) fallback to run_pnpm() for environments where pnpm and corepack are not available but vp is. - Cleanup script now uses process group kills (kill -TERM -PID) and a port-based lsof fallback to handle orphaned child processes from vp/pnpm exec wrappers. - Error cleanup trap now dumps the last 80 lines of the build log and last 40 lines of the server log to stderr, so failures are visible in CI output (previously only showed file sizes). - Test job uses run-install: false since the workspace is already built in the cache from the build job. Tested locally: deploy script succeeds, curl returns valid HTML, logs script outputs all three required markers (BUILD_ID, DEPLOYMENT_ID, NEXT_SUPPORTS_IMMUTABLE_ASSETS), cleanup kills the server. * fix: use vinext init in deploy script, add next.config.js to CJS rename list Replace the manual type:module / vite.config.ts / CJS rename logic in e2e-deploy.sh with a single `vinext init --skip-check --force` call. This runs after pnpm install (so deps are available) and before vinext build. Also add next.config.js to the CJS_CONFIG_FILES list in project.ts. Without this, vinext init adds type:module but doesn't rename CJS next.config.js files, causing 'module is not defined in ES module scope' errors. This was the root cause of the CI failures — Next.js test fixtures use module.exports in next.config.js. All 61 init tests pass. * fix: convert CJS next.config.js to ESM instead of renaming to .cjs Next.js doesn't support next.config.cjs, so we can't rename it. Instead, convert module.exports/require() to export default/import in-place after vinext init adds "type": "module". Also revert adding next.config.js to CJS_CONFIG_FILES — vinext init should not rename it since it's not a generic config file. Tolerate pnpm 10+ ERR_PNPM_IGNORED_BUILDS exit code 1 on install (the install completes, packages are in node_modules, but pnpm exits non-zero due to unapproved build scripts). Verify node_modules/vinext exists as a safety check. Tested against the real Next.js actions-streaming fixture: CJS next.config.js converted, app builds, server starts, HTTP 200. * fix: emit both IMMUTABLE_ASSET_TOKEN and NEXT_SUPPORTS_IMMUTABLE_ASSETS v16.2.x parses IMMUTABLE_ASSET_TOKEN, canary renamed it to NEXT_SUPPORTS_IMMUTABLE_ASSETS. Emit both for cross-version compat. * fix: enable JSX in .js files, convert CJS next.config.ts to ESM Add vinext:jsx-in-js transform plugin to handle JSX in plain .js files. Next.js allows JSX in .js files (Babel/SWC handle it transparently), but Vite 8's built-in vite:oxc plugin excludes .js files by default (exclude: /\.js$/) AND infers lang: 'js' from the extension (which disables JSX parsing). Neither can be fixed via config alone. The plugin runs with enforce: 'pre' before vite:oxc, transforming .js files with OXC using lang: 'jsx' so JSX is compiled before vite:oxc sees the code. This unblocks ~60 test suites in the deploy suite. Also extend the deploy script's CJS-to-ESM config converter to handle next.config.ts files (not just .js), fixing ~6 more test failures. Tested: 1284 tests pass (jsx-in-js + shims + routing + app-router), zero regressions. * ci: trigger clean workflow run * fix: improve CJS-to-ESM converter for next.config files The naive regex converter missed two patterns: - require('mod')(args) produced invalid ESM (import X from 'mod'(args)) - require() calls inside function bodies were not converted New converter handles: - module.exports = X → export default X - const X = require('mod') → import X from 'mod' - const X = require('mod')(args) → import _X from 'mod'; const X = _X(args) - require('mod') in expressions → (await import('mod')).default Tested against bundle-analyzer, path, and inline require patterns. * . * Apply suggestion from @james-elicx * fix: handle destructured requires in CJS converter, read port from file in cleanup - Add pattern for const { a, b } = require('mod') → import { a, b } from 'mod' - Read port from PORT_FILE in cleanup_on_error trap instead of using $PORT variable which may be unset if the script fails before port allocation - Add TODO noting remaining CJS edge cases (dynamic require, require.resolve) * Apply suggestions from code review Co-authored-by: James Anderson <james@eli.cx> * Apply suggestion from @james-elicx
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.