Skip to content

fix(pages): normalize data routes for trailing slash middleware - #2470

Merged
james-elicx merged 3 commits into
mainfrom
codex/fix-pages-data-trailing-slash
Jul 2, 2026
Merged

fix(pages): normalize data routes for trailing slash middleware#2470
james-elicx merged 3 commits into
mainfrom
codex/fix-pages-data-trailing-slash

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • normalize Pages /_next/data requests back to trailing-slash page paths before middleware and route matching see them
  • thread basePath and trailingSlash into Pages data normalization across generated Pages entries, worker/page handlers, prod server, and App Router Pages fallback
  • add focused regression coverage for trailing-slash Pages data normalization, including basePath preservation

Next.js parity

Next.js rebuilds /_next/data/<buildId>/*.json requests into page pathnames and, when middleware is present, applies nextConfig.trailingSlash before continuing request handling. This fixes the Pages middleware trailing-slash data-request rows from deploy-suite run 28478866791 / job 84413308650 without duplicating the rewrite-query/navigation/cache fixes already covered by sibling PRs.

Run 28478866791 mapping

Validation

Artifacts from local deploy-suite runs were cleaned after validation.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Jul 1, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2470
npm i https://pkg.pr.new/vinext@2470

commit: 9e275c2

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor
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
workers-cache preview production
web preview production

@ask-bonk

ask-bonk Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review: fix(pages): normalize data routes for trailing slash middleware

Reviewed all 8 changed files for correctness, dev/prod parity, and Next.js behavior. The change is solid and I found no blocking issues. Details below.

What was verified

Correctness of the core helper

  • normalizeNextDataPagePathname (pages-data-route.ts:91) correctly short-circuits when trailingSlash is falsy, when the path is root (/), or when it already ends in /. The default trailingSlash = false param means callers passing undefined (e.g. nextConfig?.trailingSlash in dev) behave safely.
  • In normalizePagesDataRequest, the slash is applied to pagePathname before re-adding basePath (addBasePathToPathname(pagePathname, basePath)), so /root/about/ is produced rather than /root/about. The added test at tests/pages-data-route.test.ts covers this basePath+trailingSlash interaction.

Dev/prod parity (all four request paths updated)

  • index.ts:4534 (dev middleware server) — uses nextConfig?.trailingSlash, which is the correct config field (consistent with existing usages at 4489/4592/4724).
  • prod-server.ts:1792 (Pages prod) — trailingSlash is in scope from vinextConfig?.trailingSlash ?? false at line 1578.
  • pages-page-handler.ts:313 — passes vinextConfig.basePath and vinextConfig.trailingSlash from the typed VinextConfigSubset (trailingSlash: boolean at line 86).
  • app-rsc-handler.ts:1247 (App Router Pages fallback) — passes "" for basePath, which is correct because the candidate URL already has basePath stripped at line 1240. options.trailingSlash is a defined option field (line 320) and is populated by the generated entry (__trailingSlash).

No downstream regressions in the no-middleware case
Since the normalization is now applied unconditionally (not gated on middleware presence), I checked the affected downstream consumers:

  • Route matching: matchRouteWithTrie (route-matching.ts:51) strips the trailing slash before matching, so /ssr-page/ still resolves to the /ssr-page route.
  • ISR cache keys: isrCacheKeynormalizeCachePathname (isr-cache.ts:346) strips the trailing slash, so cache keys are invariant to the added slash and won't split entries.

Generated-entry template hoisting
normalizeDataRequest in pages-server-entry.ts:240 references vinextConfig (a const declared later at line 251). This is safe: it's a hoisted function declaration executed only at request time, well after module init — matching the existing pattern at line 329.

Non-blocking observation (pre-existing, not introduced here)

pages-page-handler.ts:339-345 has a fallback that recovers dataRequestPathname from the raw originalRequestUrl.pathname via parseNextDataPathname and does not apply normalizeNextDataPagePathname. When an upstream stage already normalized the request URL to /page/ (so initialDataNorm.isDataReq is false) but the caller set options.isDataReq, this fallback can produce a trailing-slash-less asPath (router.asPath). This path pre-exists on main (confirmed via git show main:...), so it is not a regression from this PR, but it's a minor asPath parity gap worth a follow-up if trailing-slash asPath fidelity matters.

Validation performed locally

  • vp test run tests/pages-data-route.test.ts tests/entry-templates.test.ts64 passed
  • vp check on all 6 changed source files — pass (format, lint, types)

LGTM.

@ask-bonk

ask-bonk Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2470. Summary of findings:

No blocking issues found

The change correctly threads basePath and trailingSlash into Pages data normalization across all four request-handling paths (dev index.ts, Pages prod prod-server.ts, pages-page-handler.ts, and the App Router Pages fallback in app-rsc-handler.ts), maintaining dev/prod parity per the repo's guidance.

Key verifications:

  • The normalizeNextDataPagePathname helper safely short-circuits on falsy trailingSlash, root /, and already-slashed paths.
  • Slash is applied before re-adding basePath, so /root/about/ is produced correctly (covered by the new test).
  • Each call site has the correct config field in scope (nextConfig?.trailingSlash, vinextConfig.trailingSlash, options.trailingSlash).
  • The App Router fallback passes "" for basePath because the URL is already stripped upstream — correct.
  • No no-middleware regression: since normalization is now unconditional, I confirmed route matching (matchRouteWithTrie) and ISR cache keys (normalizeCachePathname) both strip trailing slashes, so /ssr-page/ still matches and cache keys don't split.
  • The generated-entry const reference (vinextConfig used before its declaration in the template) is safe due to function-declaration hoisting + request-time execution.

One non-blocking observation: a pre-existing fallback at pages-page-handler.ts:339-345 recovers dataRequestPathname without applying the trailing-slash normalization — a minor asPath parity gap that predates this PR (not a regression).

Local validation: 64 tests passed, vp check clean on all changed files.

github run

@james-elicx
james-elicx marked this pull request as ready for review July 1, 2026 08:43
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review: fix(pages): normalize data routes for trailing slash middleware

Re-reviewed at commit e91f972, which added a second commit (fix(pages): gate data trailing slash normalization on middleware) on top of the version the earlier ask-bonk comment reviewed. The gating commit materially changes the behavior, so I re-verified against Next.js source. No blocking issues — the change is now more correct than the un-gated version.

The gating commit is a genuine parity fix (verified against Next.js source)

The earlier review verified un-gated (always-on) normalization. The follow-up commit gates it on middleware presence. I confirmed this matches Next.js exactly. In base-server.ts handleNextDataRequest:

// ensure trailing slash is normalized per config
if (middleware) {
  if (this.nextConfig.trailingSlash && !pathname.endsWith('/')) {
    pathname += '/'
  }
  if (!this.nextConfig.trailingSlash && pathname.length > 1 && pathname.endsWith('/')) {
    pathname = pathname.substring(0, pathname.length - 1)
  }
}

Next.js only touches the data-request pathname's trailing slash when middleware is present. The un-gated version (commit 1) would have diverged for no-middleware apps; the gate fixes that. The new pages-page-handler.test.ts regression test asserting resolvedUrl/asPath stay /about (no slash) for the no-middleware trailingSlash: true case correctly encodes this.

Middleware-presence signal is consistent across all four paths

Each path gates on the correct, equivalent signal:

  • pages-server-entry.ts:245 / pages-page-handler.ts:317 / prod-server.ts:1794hasMiddleware && vinextConfig.trailingSlash (hasMiddleware is serverEntry.hasMiddleware === true, embedded Boolean(middlewarePath)).
  • index.ts:4538 (dev) — capturedMiddlewarePath !== null && nextConfig?.trailingSlash === true. middlewarePath is string | null, so this is a correct presence check (matches existing usage at index.ts:4737).
  • app-rsc-handler.ts:1251typeof options.runMiddleware === "function" && options.trailingSlash. Verified runMiddleware is only injected into the generated options object when middlewarePath is truthy (app-rsc-entry.ts:1185), so this is exactly equivalent to "middleware present" and consistent with Boolean(options.runMiddleware) used at app-rsc-handler.ts:492. options.trailingSlash is typed boolean, so no undefined leaks through.

capturedMiddlewarePath hoist is safe

The dev-server refactor moves const capturedMiddlewarePath = middlewarePath from ~4657 up to index.ts:4470. middlewarePath is only ever assigned once during setup (index.ts:1710), never re-assigned inside the request handler, so hoisting the capture cannot make the value stale. All downstream uses (4538, 4661, 4681, 4737) are after 4470.

Generated-entry hoisting is safe

normalizeDataRequest (pages-server-entry.ts:240) references hasMiddleware and vinextConfig, both consts declared later (248, 251). Safe: it's a hoisted function declaration invoked only at request time, after module init — same pattern already used for vinextConfig.

Non-blocking observations (pre-existing, not introduced here)

  1. trailingSlash: false removal direction is not implemented. Next.js's block also strips a trailing slash from the data pathname when trailingSlash: false + middleware. normalizeNextDataPagePathname (pages-data-route.ts:91) only handles the add direction. In practice parseNextDataPathname rarely yields a trailing-slash page segment, so this is an edge case, but it is a parity gap worth a follow-up. Not a regression — the removal direction never existed.
  2. The pages-page-handler.ts:339-345 asPath fallback gap noted in the earlier review still stands and still pre-dates this PR.

Validation performed locally

  • vp test run tests/pages-data-route.test.ts tests/entry-templates.test.ts tests/pages-page-handler.test.ts88 passed
  • vp check on all 6 changed source files — pass (format, lint, types)

LGTM.

@ask-bonk

ask-bonk Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2470. Summary:

No blocking issues found

I re-reviewed at commit e91f972, which is important because it contains a second commit that the earlier ask-bonk review didn't cover — it gates the trailing-slash data normalization on middleware presence, materially changing behavior.

Key finding — the gating commit is a genuine parity fix. I verified against Next.js base-server.ts handleNextDataRequest, which only normalizes the data pathname's trailing slash if (middleware). The original un-gated version (commit 1, the one the prior review LGTM'd) would have diverged from Next.js for no-middleware apps. The gate corrects this, and the new pages-page-handler.test.ts test encodes the no-middleware behavior correctly.

Verified correctness:

  • All four paths use an equivalent, correct middleware-presence signal (hasMiddleware, capturedMiddlewarePath !== null, typeof options.runMiddleware === "function"). Confirmed runMiddleware is only injected when middlewarePath is truthy, so it's exactly equivalent to the other paths.
  • The capturedMiddlewarePath hoist in the dev server is safe (middlewarePath is never reassigned in the request handler).
  • The generated-entry const-before-use is safe (hoisted function + request-time execution).
  • 88 tests pass; vp check clean on all changed files.

Two non-blocking, pre-existing gaps (not regressions): the trailingSlash: false slash-removal direction isn't implemented in normalizeNextDataPagePathname, and the asPath fallback gap from the earlier review still stands.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

Deploy-suite run 28478866791 / report job 84413308650 triage: this PR appears to cover the Pages middleware trailing-slash/data-route behavior rows from test/e2e/middleware-trailing-slash/test/index.test.ts.

Exact rows:

  • Middleware Runtime trailing slash > should have correct query values for rewrite to ssg page
  • Middleware Runtime trailing slash > should have correct dynamic route params for middleware rewrite to dynamic route
  • Middleware Runtime trailing slash > should have correct route params for chained rewrite from middleware to config rewrite
  • Middleware Runtime trailing slash > should trigger middleware for data requests
  • Middleware Runtime trailing slash > should keep non data requests in their original shape
  • Middleware Runtime trailing slash > should add a rewrite header on data requests for rewrites
  • Middleware Runtime trailing slash > allows shallow linking with middleware

Since this PR is already James-owned, I am skipping duplicate implementation work for these rows.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 9e275c2 against base 239595e using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 125.6 KB 125.6 KB ⚫ 0.0%
Client entry size (gzip) vinext 119.8 KB 119.8 KB ⚫ 0.0%
Dev server cold start vinext 2.89 s 2.88 s ⚫ -0.4%
Production build time vinext 3.36 s 3.39 s ⚫ +1.1%
RSC entry closure size (gzip) vinext 96.8 KB 96.9 KB ⚫ +0.0%
Server bundle size (gzip) vinext 163.2 KB 163.2 KB ⚫ +0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx
james-elicx merged commit 9e1866a into main Jul 2, 2026
63 checks passed
@james-elicx
james-elicx deleted the codex/fix-pages-data-trailing-slash branch July 2, 2026 19:04
@github-actions github-actions Bot mentioned this pull request Jul 2, 2026
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.

1 participant