fix: return route pattern from router.pathname, not resolved path#463
Conversation
Move Cloudflare instrumentation example state from module-level variables to globalThis so startup and request paths share one owner across different module instances. Add waitForHydration() helper to Pages Router E2E tests to eliminate flaky button-click assertions that raced against React hydration.
The `as` parameter in push() and replace() was silently ignored (prefixed as _as) in both the useRouter() hook and the Router singleton. This broke masked URLs and legacy dynamic-route flows. Extract resolveNavigationTarget() helper that uses `as` when provided, falling back to resolveUrl(url) otherwise. Apply to all four call sites: useRouter().push, useRouter().replace, Router.push, Router.replace.
- Add doc comment to resolveNavigationTarget explaining the url/as simplification vs Next.js's two-value approach - Extract waitForHydration to shared tests/e2e/helpers.ts - Add pathname assertion to verify router.pathname stays as route pattern (/posts/[id]) after as-based navigation
router.pathname currently returns the resolved path after client-side navigation, not the route pattern. The correct behavior (returning /posts/[id] instead of /posts/42) requires the two-value url/as approach tracked in cloudflare#462.
In Next.js, router.pathname returns the route pattern (e.g.,
"/posts/[id]"), not the resolved path ("/posts/42"). vinext was
deriving pathname from window.location.pathname which gives the
resolved path.
Fix: use __NEXT_DATA__.page (which holds the route pattern) as
pathname, falling back to location.pathname when __NEXT_DATA__ isn't
available. asPath continues to use the resolved browser path.
Closes cloudflare#462
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 090b76cbc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // In Next.js, router.pathname is the route pattern (e.g., "/posts/[id]"), | ||
| // not the resolved path ("/posts/42"). __NEXT_DATA__.page holds the route | ||
| // pattern and is updated by navigateClient() on every client-side navigation. | ||
| const pathname = window.__NEXT_DATA__?.page ?? resolvedPath; |
There was a problem hiding this comment.
Switching client pathname to window.__NEXT_DATA__.page here makes dynamic-route pages hydrate with a different value than the server markup, because SSR still seeds router state from the resolved URL path (e.g. setSSRContext({ pathname: routeUrl.split("?")[0], ... }) in the pages server entries). When a page renders router.pathname in HTML (like /posts/[id] pages), SSR prints /posts/123 but hydration computes /posts/[id], which triggers hydration mismatch warnings and DOM replacement on first load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 509e340. All three SSR entry points (dev-server, pages-server-entry, static-export) now use patternToNextFormat(route.pattern) for the SSR context pathname, matching what __NEXT_DATA__.page already provides. SSR and hydration are now consistent.
All three SSR entry points (dev-server, pages-server-entry, static-export) were setting pathname to the resolved URL path (e.g., "/posts/42") while the client now derives it from __NEXT_DATA__.page (the route pattern "/posts/[id]"). This caused hydration mismatches when components render router.pathname. Fix: use patternToNextFormat(route.pattern) for the SSR context pathname, matching what __NEXT_DATA__.page already provides.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix router.pathname semantics
This is a solid fix for a real Next.js compatibility issue. The change correctly makes router.pathname return the route pattern (/posts/[id]) instead of the resolved path (/posts/42), matching Next.js semantics.
What's good
- Consistent SSR/hydration: All three SSR entry points (
dev-server.ts,pages-server-entry.ts,static-export.ts) now usepatternToNextFormat(route.pattern)for the SSR context pathname. This avoids the hydration mismatch that the Codex bot flagged (and the author fixed in this commit). - Clean separation of
pathnamevsasPath:getPathnameAndQuery()correctly uses__NEXT_DATA__.pageforpathname(route pattern) and the actual browser URL forasPath. resolveNavigationTargethelper: Good extraction — theasparameter handling is clear and the JSDoc accurately describes the limitation (purely decorativeasvalues aren't supported).- Test coverage: New E2E tests for
push(url, as)andRouter.replace(url, as)with proper assertions on bothpathnameandasPath. waitForHydrationshared helper: Good consolidation. Though note the codebase still has many inline copies of this pattern (see minor comment below).- instrumentation-state.ts refactor: The
globalThis-based approach is the right fix for cross-module-instance state in Cloudflare Workers.
Minor observations (non-blocking)
A few things worth noting but none are blockers.
|
|
||
| export async function waitForHydration(page: Page) { | ||
| await page.waitForFunction(() => Boolean((window as any).__VINEXT_ROOT__)); | ||
| } |
There was a problem hiding this comment.
Nice to have this as a shared helper. There are ~15+ other E2E spec files that still define their own local waitForHydration or inline page.waitForFunction(() => (window as any).__VINEXT_ROOT__). Consolidating those to use this shared import could be done in a follow-up to reduce duplication.
| as: string | undefined, | ||
| locale: string | undefined, | ||
| ): string { | ||
| return applyNavigationLocale(as ?? resolveUrl(url), locale); |
There was a problem hiding this comment.
The comment above accurately describes the limitation: as must be a server-resolvable path because vinext fetches HTML from it. Worth noting that in Next.js, router.push({ pathname: '/posts/[id]', query: { id: '42' } }) (without as) resolves the dynamic segments automatically. Here, without as, resolveUrl would produce /posts/[id]?id=42 which isn't a valid server path.
This is a known gap and the comment documents it, but it could catch users who rely on the pattern-only form without as. Consider a follow-up to resolve dynamic segments from the UrlObject.pathname when as is omitted (match /posts/[id] + query.id → /posts/42).
| // In Next.js, router.pathname is the route pattern (e.g., "/posts/[id]"), | ||
| // not the resolved path ("/posts/42"). __NEXT_DATA__.page holds the route | ||
| // pattern and is updated by navigateClient() on every client-side navigation. | ||
| const pathname = window.__NEXT_DATA__?.page ?? resolvedPath; |
There was a problem hiding this comment.
The fallback to resolvedPath when __NEXT_DATA__ isn't available is sensible — it handles edge cases where the page loaded without the SSR data script (e.g., a raw HTML shell). Good defensive coding.
|
Review submitted. Here's a summary of my findings: Approved — This is a correct and well-tested fix for Key changes reviewed
Minor notes left on the PR
|
…-value # Conflicts: # tests/e2e/pages-router/navigation.spec.ts
2 participants
Summary
router.pathnamenow returns the route pattern (e.g.,/posts/[id]) instead of the resolved path (/posts/42), matching Next.js semanticsrouter.asPathcontinues to return the resolved browser URL as expected__NEXT_DATA__.page(which holds the route pattern from SSR and is updated on every client navigation) as the source of truth forpathnameDepends on #453 — this branch includes those commits. Merge #453 first, then this PR.
Closes #462
Test plan
shims.test.ts)router.pathnameis/posts/[id]afteras-based navigationrouter.pathnameis/posts/[id]on SSR page load