fix(router): honor history state url on popstate#2116
Draft
NathanDrake2406 wants to merge 8 commits into
Draft
Conversation
Pages Router popstate handling previously derived the route to load from window.location. That loses the route URL stored in Next-shaped history state when url and as differ, so a stale Safari replay is ignored correctly but the next identical popstate still reloads the visible static route instead of the dynamic page route. Use state.as as the visible URL and state.url as the route fetch target for masked popstate entries, then force the HTML navigation path so data fetching is not keyed by the browser URL. Port the upstream Next.js ignore-invalid-popstateevent without-i18n coverage to lock in the stale-state behaviour.
commit: |
main's cloudflare#2076 ("enhance navigation with `as` mask") landed an independent fix for the same masked-popstate bug this branch targets, threading a `routeUrl` through runNavigateClient/navigateClient and passing raw {url, as, options} to beforePopState (true Next.js parity). That implementation supersedes this branch's `forceHtmlFetch` / resolvePopStateNavigationTarget approach, so the router.ts conflict is resolved in favour of main's design. main still had a residual gap: a masked popstate whose visible URL is unchanged (the exact without-i18n / with-i18n e2e scenario, where `as` is the static mask on both entries) was classified as hash-only via `browserUrl === lastPathnameAndSearch` and short-circuited before the route fetch. Next.js's onlyAHashChange returns false for an identical no-hash `as`, so it falls through to a full navigation. Hoist main's stateRouteUrl computation above the hash-only check, derive isMaskedRoute from it, and exclude masked entries from the hash-only fast path. Keep this branch's ported regression tests (first stale event ignored, second fetches state.url) and add the i18n and basePath companions the review flagged as missing. beforePopState masked coverage already exists on main via cloudflare#2076, so it is not duplicated here.
Contributor
Performance benchmarksCompared 0 improved · 4 regressed · 2 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% |
The new i18n / basePath popstate tests typed their fetch mock and history state loosely, so `vp check` flagged the `mock.calls[0]` index and the state literals carried `any`. Type the fetch mock with its call signature, and introduce `MaskedHistoryState` / `PopStateListener` aliases so the ported popstate fixtures and the handler invocations are checked.
The masked-popstate tests construct `MaskedHistoryState` by hand and drive the captured popstate handler in isolation. That proves the consumer reads `state.url`, but nothing exercises the writer, so a future change to `updateHistory`/`Router.push` that stopped emitting `url` distinct from `as` would pass every existing test while breaking masked forward-navigation. Add a producer→consumer integration test: a masked `Router.push(href, as)` writes real history state via `updateHistory`, and that exact captured object is replayed through popstate. It asserts the producer wrote `url !== as` and the consumer fetched `state.url` (the route), not `state.as` (the address bar). The test reproduces the PR's bug through the real write path: after the push the router tracker `lastPathnameAndSearch` equals the visible URL, so the masked entry lands on the identical-URL case that the `isMaskedRoute` guard excludes from the hash-only fast path. Verified red on the pre-guard `isHashOnly` (0 fetches), green with the guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Pages Router popstate now tracks the previous full browser URL separately from the existing hash-stripped
lastPathnameAndSearchtracker. The popstate hash-only check uses the sharedisHashOnlyBrowserUrlChange(window.location.href, lastBrowserUrl, __basePath)helper, matching the push path and avoiding the hash-blindpathname + searchcomparison.stateRouteUrlis still used as the masked route fetch target whenstate.url !== state.as; the masked-route-only guard is gone.Why
lastPathnameAndSearchis intentionally hash-stripped and is still needed that way for the Safari replay filter and push-to-replace coercion. Using it for popstate hash-only detection made identical no-hash URLs look hash-only becausebrowserUrl === lastPathnameAndSearchcould be true without any hash delta.That was visible in
test/e2e/ignore-invalid-popstateevent: both popstate events can keep the address bar on the same static mask (as: "/static") whilestate.urlchanges to the dynamic route behind it. The old comparison short-circuited the second event as hash-only and skipped the route fetch. Next.js'sonlyAHashChangedoes not treat an identical no-hashasas hash-only, so it falls through to the full navigation.CI also caught the companion traversal case: browser back from
/#hashto/removes the destination hash but is still hash-only in Next.js. The shared helper now treats same path/search with either a hash-bearing destination or a changed previous hash as hash-only, while still returning false for identical no-hash URLs.Approach
lastBrowserUrlto Pages Router runtime state and initialize it fromwindow.location.href.lastPathnameAndSearchis updated after history writes or accepted popstate navigation.browserUrl === lastPathnameAndSearchhash-only check withisHashOnlyBrowserUrlChange(window.location.href, lastBrowserUrl, __basePath).lastPathnameAndSearchhash-stripped for the existing stale-event and push/replace comparisons.stateRouteUrlas the masked fetch target forrunNavigateClient.updateBrowserUrlTrackers().Validation
vp test run tests/pages-router-i18n-sticky-locale.test.tsvp test run tests/shims.test.ts -t "isHashOnlyBrowserUrlChange|popstate|hash-only"PLAYWRIGHT_PROJECT=pages-router pnpm run test:e2e -- tests/e2e/pages-router/router-events.spec.tsI also checked
server/prod-server.tsandserver/dev-server.tsfor equivalent Pages Router popstate/hash classification logic to sync; this path is client-side in the Pages Router shim, with no production server counterpart.