Skip to content

[codex] Add first-pass Pages Router deploy suite parity#922

Draft
southpolesteve wants to merge 18 commits into
mainfrom
codex/pages-router-suite-main
Draft

[codex] Add first-pass Pages Router deploy suite parity#922
southpolesteve wants to merge 18 commits into
mainfrom
codex/pages-router-suite-main

Conversation

@southpolesteve
Copy link
Copy Markdown
Collaborator

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

  • Adds compatibility fixes for Pages Router SSR/data routes, API routes, routing state, middleware/header behavior, i18n/basePath/trailingSlash handling, and several asset/runtime edge cases surfaced by the Next.js deploy suites.
  • Adds focused local tests for the new compatibility seams, including router behavior, Pages data responses, API routes, middleware, static asset compatibility, server externals, WASM, CSS data URLs, import.meta.url, styled-jsx, SWC helpers, and trace metadata.
  • Adds a manual-only GitHub Actions workflow, Next.js Pages Router Deploy Suite, triggered with workflow_dispatch and sharded 16 ways. This is deliberately on-demand only and should not run on every PR.
  • Adds a manifest generator that derives a Pages Router-only deploy manifest from Next.js test/deploy-tests-manifest.json, excluding test/e2e/app-dir/** so canary drift does not silently expand the scope.

Validation

  • vp check passed after rebasing onto latest origin/main (598782e), with warning-only lint/type output.
  • vp run vinext#build passed.
  • Manifest dry run selected exactly 207 suites and 0 app-dir suites.
  • Full local Pages Router deploy-suite verification passed before PR prep: 207/207 suites passing.
  • Post-rebase compact canary passed on retry 0/2: module-layer, og-api, getserversideprops, and opentelemetry/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.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 27, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@922

commit: b2bc83e

@github-actions
Copy link
Copy Markdown

Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original

@southpolesteve
Copy link
Copy Markdown
Collaborator Author

/bigbonk review this please

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/...json redirect shape from getStaticProps / getServerSideProps (packages/vinext/src/server/pages-page-data.ts:376-384 and :525-532): For data requests, vinext returns a plain 307/308 with a Location header. Next.js's data layer expects either x-nextjs-redirect: <dest> (modern) or a JSON body with __N_REDIRECT for the client router to perform the redirect. The middleware path in prod-server.ts:1909 already uses x-nextjs-redirect for data requests; gSSP/gSP need the same treatment. Same gap exists in dev-server.ts:538-552. A meaningful fraction of the Next.js Pages Router data-redirect deploy tests will fail until this is fixed.

  • getStaticProps / getServerSideProps notFound for data requests (pages-page-data.ts:386-396, dev-server.ts:553-565): Dev and the data path return JSON 404 / buildPagesDataNotFoundResponse for data requests, but the dev-server falls through to renderErrorPage (HTML) regardless of isDataRequest. Pages Router data requests should always return JSON {"notFound": true} (or 404 with x-nextjs-redirect style). 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.

  • matchConfigPattern case-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: any redirects/rewrites source 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): Adding pattern.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 through javascript: to window.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.ts and dev-server.ts skew in middleware-driven path normalization: prod-server.ts:1853-1857 builds reqCtx before middleware runs (so config redirects/headers see the original request), while dev-server.ts (via index.ts:2916-2918) takes a similar pre-middleware snapshot. Both look correct after this PR — good. One nit: in prod, requestRedirects.length is computed every request from rulesForRequestBasePath(...) (allocating a filtered array even when nothing matches). For high-RPS workloads this could be cached at config-load time keyed by requestHadBasePath. Not blocking.

  • mergeWebResponse and streamed-response Content-Length (prod-server.ts:265-312): Correctly preserves Set-Cookie array-style headers via getSetCookie() and strips Content-Length for streamed HTML. The 204/205/304 body-drop path correctly cancels the upstream body stream. Looks good.

  • pages-page-response.ts:414 sets Content-Type: text/html unconditionally inside applyGsspHeaders. Verified upstream that this helper is only called on the non-data render path; data requests construct their own application/json headers in the entry. OK, but a comment on applyGsspHeaders saying "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/:slug to verify the casing of the captured :slug is preserved.

  • edge-blob-assets.ts:17-23: isExternalSpecifier uses new URL(specifier) and considers anything with a non-empty protocol external. This will treat Windows drive-letter paths like C:\foo.png as having protocol === "c:" and skip them. On Windows that means import 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:73 uses output.replaceAll(fullMatch, ...) which works because the literal new 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 to MagicString for consistency with import-meta-url.ts and proper sourcemaps.

  • import-meta-url.ts: Looks careful — skips strings, comments, and new URL(..., import.meta.url) bases. The findEnclosingParen look-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:50 uses new WebAssembly.Module(bytes) synchronously at module evaluation. On Cloudflare Workers, large WASM modules hit a 50 ms CPU cap on synchronous compilation; consider WebAssembly.compile() (async). Not blocking — the existing comment correctly notes the Cloudflare Vite plugin already handles ?module natively 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. The findPackageJsonFromResolvedFile walk has a correct termination condition (parent === dir).

  • Form shim ordering (shims/form.tsx:363-364): e.preventDefault() is called after createFormSubmitDestinationUrl(). If new URL(action, window.location.href) throws, the form will submit natively. A defensive try/catch here would prevent silent failure modes. Minor.

  • pages-server-entry.ts middleware code generation (:212-231): Builds a NextRequest with __mwNextConfig to expose nextUrl.basePath and nextUrl.locale. Looks right. Note the mwRequest = 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 as NextRequest at 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 / __renderPagesPageResponse repeated across multiple snapshot fixtures, which is fine and consistent with AGENTS.md guidance. The one piece of substantive new logic added directly to entries/app-rsc-entry.ts is the x-vinext-loading-payload === "1" handler (renders loading.tsx to an RSC stream and returns it). That's ~25 lines of renderToReadableStream call, header construction, and a 204 fallback — borderline. Consider extracting it to server/app-loading-handler.ts so 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 _renderPage function's local i18n + query-derivation block (lines 859-967) is cohesive enough to keep inline, but if it grows further a server/pages-render-context.ts helper 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:1015 regex /(?:^|\/)pages[-.]/ for finding the Pages client entry could match unrelated assets named xpages-* if they ever existed. Anchor with ^ or / is already there but the pattern is loose.
  • config-matchers.ts:96 _LOCALE_STATIC_RE doesn't tolerate trailing / in the suffix; mostly fine but worth a brief comment.
  • nextjs-deploy-suite-deploy.sh:188 sets IMMUTABLE_ASSET_TOKEN="undefined" (the literal string "undefined") and then never uses it. Looks like dead code from porting; remove.
  • nextjs-deploy-suite-deploy.sh:51 BUILD_LOG=".vinext-deploy-build.log" paths are relative to the test app cwd. cleanup.sh uses 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, 350 print 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.yml is correctly workflow_dispatch-only with permissions: contents: read and no secret usage. ✅
  • scripts/run-nextjs-deploy-suite.sh correctly uses set -euo pipefail and quotes all variable expansions. ✅
  • Tests in tests/router-scroll-restoration.test.ts look non-tautological — they exercise sessionStorage round-trips, scrollRestoration mode, and popstate handling against a fake window with proper getter/setter semantics. The import("...?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.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 27, 2026

The review has been posted to PR #922.

Summary

I 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:

  • _next/data redirect format gap in pages-page-data.ts:376-384,525-532 and dev-server.ts:538-552 — vinext emits a plain Location: redirect on data requests instead of Next.js's x-nextjs-redirect header / __N_REDIRECT JSON body. This will fail the very Next.js Pages Router deploy tests this PR targets.
  • Dev/prod skew on notFound data requests: prod handles correctly, dev-server.ts:553-565 falls through to HTML error path.

Parity / maintainability:

  • Config-headers-before-middleware gap from AGENTS.md is perpetuated (dev/prod at least stay consistent).
  • App Router loading-payload renderToReadableStream wiring (~25 lines) landed in a generated-entry template instead of being extracted into server/*, contrary to the AGENTS.md "generated entries stay thin" guidance.
  • config-matchers.ts:722,773 introduces case-insensitivity for matchers — appears to move toward Next.js parity but worth a regression note.

Verified safe:

  • Workflow is correctly workflow_dispatch-only with no secret exposure.
  • Shell scripts use set -euo pipefail with quoted expansions.
  • New plugins (css-data-url, edge-blob-assets, import-meta-url, wasm-module, swc-helpers, styled-jsx) are clean — no path traversal, no Node-API leaks into Worker bundles.
  • mergeWebResponse/mergeResponseHeaders preserves multi-value Set-Cookie via getSetCookie().
  • javascript: URI handling in Link/Form/router unchanged from prior PR fix(app-router): block javascript: URLs in router.push/replace/prefetch #906 and matches Next.js's intentional pass-through.
  • server-externals.ts is correctly build-time/Node-only scoped.
  • Scroll restoration tests are non-tautological.

github run

@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk review

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 29, 2026

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

james-elicx added a commit that referenced this pull request May 14, 2026
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.
james-elicx added a commit that referenced this pull request May 14, 2026
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.
james-elicx added a commit that referenced this pull request May 14, 2026
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants