fix: honor the as parameter in router.push/replace#453
fix: honor the as parameter in router.push/replace#453james-elicx merged 4 commits intocloudflare:mainfrom
as parameter in router.push/replace#453Conversation
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.
commit: |
|
/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: as parameter support in router.push/replace
The core fix is correct — as was being silently ignored via _as prefix, and the resolveNavigationTarget() helper is a clean factoring. The E2E tests cover the common case well, and the hydration/instrumentation fixes are sensible improvements.
One architectural concern about how as is handled, and some smaller notes below.
Semantic gap with Next.js as
In Next.js, url and as are tracked as two separate values throughout the navigation lifecycle:
urldetermines which page module to load and which data to fetchasis the cosmetic URL shown in the browser bar- Both are stored in
history.statefor back/forward restoration
This PR collapses them into a single value: when as is provided, url is completely discarded. The returned value is used for both window.history.pushState({}, "", full) and navigateClient(full). This works for the common case tested here (where as is a concrete URL that the server can route to the same page), but would break if as were a purely decorative URL that doesn't map to the real route.
I think this is acceptable for now — the truly-decorative-URL pattern was largely replaced by rewrites after Next.js 9.5.3. But it's worth documenting the limitation. See inline comments.
waitForHydration duplication
The same waitForHydration helper is copy-pasted into both hydration.spec.ts and navigation.spec.ts. Worth extracting to a shared location (e.g., tests/e2e/fixtures.ts or a new tests/e2e/helpers.ts) to avoid drift.
| function resolveNavigationTarget( | ||
| url: string | UrlObject, | ||
| as: string | undefined, | ||
| locale: string | undefined, | ||
| ): string { | ||
| return applyNavigationLocale(as ?? resolveUrl(url), locale); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the crux of the fix and it handles the common case correctly. However, it collapses url and as into a single value, whereas Next.js keeps them separate: url for data fetching/page resolution, as for the browser URL bar.
When as is provided, url is entirely discarded. The result is used for both history.pushState and navigateClient(). If someone passes an as that doesn't correspond to a real server route (a purely cosmetic URL), navigateClient would fetch the wrong page or 404.
This is fine for the overwhelmingly common case (and the pattern shown in the tests), but a comment documenting the simplification would help future contributors:
| function resolveNavigationTarget( | |
| url: string | UrlObject, | |
| as: string | undefined, | |
| locale: string | undefined, | |
| ): string { | |
| return applyNavigationLocale(as ?? resolveUrl(url), locale); | |
| } | |
| function resolveNavigationTarget( | |
| url: string | UrlObject, | |
| as: string | undefined, | |
| locale: string | undefined, | |
| ): string { | |
| // When `as` is provided, use it as the navigation target. This is a | |
| // simplification: Next.js keeps `url` and `as` as separate values (url for | |
| // data fetching, as for the browser URL). We collapse them because vinext's | |
| // navigateClient() fetches HTML from the target URL, so `as` must be a | |
| // server-resolvable path. Purely decorative `as` values are not supported. | |
| return applyNavigationLocale(as ?? resolveUrl(url), locale); | |
| } |
There was a problem hiding this comment.
Added the doc comment in 3da95b4. Also filed a tracking issue for the full two-value approach if we ever need it.
| ): Promise<boolean> => { | ||
| let resolved = applyNavigationLocale(resolveUrl(url), options?.locale); | ||
| async (url: string | UrlObject, as?: string, options?: TransitionOptions): Promise<boolean> => { | ||
| let resolved = resolveNavigationTarget(url, as, options?.locale); |
There was a problem hiding this comment.
Worth noting: after this line resolves to the as value, downstream navigateClient(full) fetches HTML from as, and history.pushState({}, "", full) stores only as in the URL. On back/forward (popstate), the handler reads window.location.pathname (= as) to re-fetch data. This works when as is a concrete resolvable URL but wouldn't for purely cosmetic ones.
Next.js stores both url and as in the history state object and uses url for data fetching on popstate. Consider filing a tracking issue for the full two-value approach if/when someone hits this.
| const BASE = "http://localhost:4173"; | ||
|
|
||
| async function waitForHydration(page: import("@playwright/test").Page) { | ||
| await page.waitForFunction(() => Boolean((window as any).__VINEXT_ROOT__)); |
There was a problem hiding this comment.
This same waitForHydration helper is duplicated in hydration.spec.ts:5. Consider extracting it to a shared location (e.g., tests/e2e/fixtures.ts or a new tests/e2e/helpers.ts) to avoid the copy drifting.
There was a problem hiding this comment.
Done — extracted to tests/e2e/helpers.ts in 3da95b4. There are ~20 other files with the same duplication but keeping the scope to what this PR touched.
| test("router.push(url, as) uses the masked URL while resolving the real route", async ({ | ||
| page, | ||
| }) => { | ||
| await page.goto(`${BASE}/nav-test`); | ||
| await expect(page.locator("h1")).toHaveText("Navigation Test"); | ||
| await waitForHydration(page); | ||
|
|
||
| await page.click('[data-testid="push-post-as-hook"]'); | ||
| await expect(page.locator('[data-testid="post-title"]')).toHaveText("Post: 42"); | ||
| await expect(page.locator('[data-testid="query"]')).toHaveText("Query ID: 42"); | ||
| await expect(page.locator('[data-testid="as-path"]')).toHaveText( | ||
| "As Path: /posts/42?from=hook", | ||
| ); | ||
| expect(page.url()).toBe(`${BASE}/posts/42?from=hook`); | ||
| }); |
There was a problem hiding this comment.
Good test case. One thing that would strengthen coverage: assert that router.pathname is the route pattern (/posts/[id]) rather than the resolved path (/posts/42). This is an important Next.js semantic — after navigating with as, pathname should remain the route pattern while asPath is the actual browser URL. The fixture already has a [data-testid="pathname"] element, so adding:
await expect(page.locator('[data-testid="pathname"]')).toHaveText("Pathname: /posts/[id]");would verify that vinext correctly reports the route pattern via router.pathname after an as-based navigation.
There was a problem hiding this comment.
Good catch — added await expect(page.locator('[data-testid="pathname"]')).toHaveText("Pathname: /posts/[id]"); in 3da95b4.
| test("Router.replace(url, as) uses the masked URL for singleton navigation", async ({ page }) => { | ||
| await page.goto(`${BASE}/`); | ||
| await expect(page.locator("h1")).toHaveText("Hello, vinext!"); | ||
|
|
||
| await page.goto(`${BASE}/nav-test`); | ||
| await expect(page.locator("h1")).toHaveText("Navigation Test"); | ||
| await waitForHydration(page); | ||
|
|
||
| await page.click('[data-testid="replace-post-as-singleton"]'); | ||
| await expect(page.locator('[data-testid="post-title"]')).toHaveText("Post: 84"); | ||
| await expect(page.locator('[data-testid="query"]')).toHaveText("Query ID: 84"); | ||
| await expect(page.locator('[data-testid="as-path"]')).toHaveText( | ||
| "As Path: /posts/84?from=singleton", | ||
| ); | ||
| expect(page.url()).toBe(`${BASE}/posts/84?from=singleton`); | ||
|
|
||
| await page.goBack(); | ||
| await expect(page.locator("h1")).toHaveText("Hello, vinext!"); | ||
| }); |
There was a problem hiding this comment.
Nice — testing the Router singleton path separately from the useRouter() hook path gives good coverage of all four call sites. The goBack assertion at the end verifying we land on Home (not nav-test) correctly validates the replace semantics.
| function getInstrumentationState(): InstrumentationState { | ||
| const scopedGlobal = globalThis as typeof globalThis & { | ||
| __VINEXT_CLOUDFLARE_INSTRUMENTATION_STATE__?: InstrumentationState; | ||
| }; | ||
|
|
||
| /** List of errors captured by onRequestError(). */ | ||
| const capturedErrors: CapturedRequestError[] = []; | ||
| if (!scopedGlobal.__VINEXT_CLOUDFLARE_INSTRUMENTATION_STATE__) { | ||
| scopedGlobal.__VINEXT_CLOUDFLARE_INSTRUMENTATION_STATE__ = { | ||
| capturedErrors: [], | ||
| registerCalled: false, | ||
| }; | ||
| } | ||
|
|
||
| return scopedGlobal.__VINEXT_CLOUDFLARE_INSTRUMENTATION_STATE__; | ||
| } |
There was a problem hiding this comment.
The globalThis approach is the right fix for cross-module-instance visibility, and the lazy initialization is clean.
Minor question: is there any concern about state leaking across requests in production Workers (where globalThis persists across isolate reuses)? resetInstrumentationState() exists, but is it called at the start of each request? If not, capturedErrors from a previous request could accumulate.
There was a problem hiding this comment.
No concern here — this file is only used in the example app for e2e testing, not production code. capturedErrors accumulation is intentional: tests hit routes that trigger errors, then assert on the captured list via GET /api/instrumentation-test. The DELETE endpoint exists to reset between test runs. In a real app, users wouldn't use this module.
|
Review posted successfully. Here's a summary of the feedback I left on PR #453: Core fix is correct — Key architectural feedback:
|
- 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.
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: as parameter support in router.push/replace
The core fix is correct and well-implemented. The _as prefix was silently discarding a real Next.js API parameter, and the resolveNavigationTarget() extraction is clean. The previous review from ask-bonk covered the main architectural concerns thoroughly, and the author has addressed them (doc comment added, waitForHydration extracted, tracking issue filed). The E2E tests cover both useRouter() hook and Router singleton paths with good assertions.
A few remaining observations below — none are blocking.
| "As Path: /posts/42?from=hook", | ||
| ); | ||
| // TODO(#462): after implementing two-value url/as navigation, assert that | ||
| // router.pathname stays as the route pattern (/posts/[id]) not the resolved path. |
There was a problem hiding this comment.
The TODO referencing #462 is a good way to track this. One clarification: the comment says "assert that router.pathname stays as the route pattern" — but currently, after client navigation with as, does router.pathname return the resolved path (/posts/42) or the pattern (/posts/[id])? If it returns the resolved path, that's a known behavioral gap (not just a missing assertion). It might be worth making that explicit in the issue.
Also, the [data-testid="pathname"] element is present on posts/[id].tsx — even without the TODO assertion here, navigating to this page does implicitly render router.pathname. A quick sanity check: if router.pathname currently resolves to /posts/42 instead of /posts/[id], that would also affect the asPath value (since Next.js computes asPath differently when it has both url and as). Worth documenting in the tracking issue.
There was a problem hiding this comment.
Yes, it was a behavioral gap — router.pathname was returning the resolved path (/posts/42) because getPathnameAndQuery() derived it from window.location.pathname. Fixed in #463: now uses __NEXT_DATA__.page (the route pattern) for pathname, while asPath continues using the resolved browser path. Both SSR and client-side navigation are covered.
| await expect(page.locator("h1")).not.toHaveText("Navigation Test"); | ||
| }); | ||
|
|
||
| test("Router.replace(url, as) uses the masked URL for singleton navigation", async ({ page }) => { |
There was a problem hiding this comment.
Minor: the test navigates to home first (page.goto(BASE)), then to nav-test (page.goto(BASE/nav-test)). The double goto is intentional to set up the history stack for the goBack assertion at the end, but a brief comment explaining this (similar to the one in the router.replace test at line 84) would help readability.
| test("Router.replace(url, as) uses the masked URL for singleton navigation", async ({ page }) => { | |
| test("Router.replace(url, as) uses the masked URL for singleton navigation", async ({ page }) => { | |
| // Start at home so goBack() at the end lands here (replace removes nav-test from history) | |
| await page.goto(`${BASE}/`); | |
| await expect(page.locator("h1")).toHaveText("Hello, vinext!"); |
| import type { Page } from "@playwright/test"; | ||
|
|
||
| export async function waitForHydration(page: Page) { | ||
| await page.waitForFunction(() => Boolean((window as any).__VINEXT_ROOT__)); |
There was a problem hiding this comment.
Good extraction. Note that there are ~15+ other E2E test files that still define their own local waitForHydration (e.g., shallow-routing.spec.ts, routes.spec.ts, server-actions.spec.ts, form.spec.ts, etc.). The author noted this is out of scope for this PR which is reasonable — but a follow-up to consolidate all of them would prevent future drift. Consider adding a brief comment or filing an issue to track it.
Also worth noting: this helper currently has no timeout parameter. If hydration takes longer than Playwright's default timeout (30s), the test will fail with a generic timeout error rather than a clear "hydration didn't complete" message. Not blocking, but a { timeout } option would improve debugging:
| await page.waitForFunction(() => Boolean((window as any).__VINEXT_ROOT__)); | |
| export async function waitForHydration(page: Page, timeout = 10_000) { | |
| await page.waitForFunction(() => Boolean((window as any).__VINEXT_ROOT__), { timeout }); | |
| } |
| as: string | undefined, | ||
| locale: string | undefined, | ||
| ): string { | ||
| return applyNavigationLocale(as ?? resolveUrl(url), locale); |
There was a problem hiding this comment.
Nit: when as is provided as a string like "/posts/42?from=hook", it bypasses resolveUrl() entirely — which is correct for the common case. But if someone passes as with special characters that need encoding, this could produce unexpected URLs. Next.js internally normalizes the as value. Not blocking since the same limitation existed before (just with url), but worth being aware of.
|
Review posted successfully. Here's a summary of the feedback on PR #453: Verdict: Approved — The fix is correct and the implementation is clean. The core issue was that all four call sites ( Inline comments left:
|
2 participants
Summary
push()andreplace()silently ignoring theasparameter in bothuseRouter()hook andRoutersingleton — this broke masked URLs and legacy dynamic-route flowsresolveNavigationTarget()helper: usesaswhen provided, falls back toresolveUrl(url)otherwiseuseRouter().push,useRouter().replace,Router.push,Router.replaceglobalThisfor cross-module-instance visibilitywaitForHydration()helper to Pages Router E2E tests to fix flaky hydration racesTest plan
router.push(url, as)uses masked URL while resolving the real routeRouter.replace(url, as)uses masked URL for singleton navigation