fix(i18n): make locale sticky across client navigations#1407
fix(i18n): make locale sticky across client navigations#1407james-elicx wants to merge 4 commits into
Conversation
Refs #1336 (item 2). Writes a Next.js-compatible history state shape on every router push/replace, captures the active locale in `state.options.locale`, and teaches the popstate handler to: 1. Ignore history entries written by third-party code (no `__N: true`). 2. Drop the first popstate when it carries the same locale + `as` as the current page (Safari tab-restore / BFCache replay parity). 3. Honour the locale recorded in history state when re-fetching the page on back/forward, so default-locale roots still go through their locale-qualified HTML endpoint. This matches the Next.js behaviour exercised by `test/e2e/ignore-invalid-popstateevent` and the client half of `test/e2e/i18n-preferred-locale-detection`. Scope boundary: the server-side `Set-Cookie: NEXT_LOCALE=...` on initial Accept-Language detection is intentionally NOT modified here. The existing `parseCookieLocaleFromHeader` already gives the cookie priority over Accept-Language, and changing where the cookie is written would overlap with #1336 item 4 (default-locale prefix normalisation).
commit: |
|
…hape
The previous commit attached `__N: true` to router-owned history entries,
but two regressions slipped in:
1. `saveScrollPosition` merges scroll fields onto the *current* entry
before a push. When the current entry was the initial document load
(`state === null`), the merge produced a state object without `__N`,
which the popstate foreign-state filter then dropped — breaking
browser back/forward.
Fix: when the existing state is null, mint a router-shaped state
(`{ __N: true, url, as, options, key }`) before merging the scroll
fields, so every entry the router touches carries `__N`.
2. The Safari-replay filter compared `state.as` against `window.location`,
but `window.location` has already changed by the time popstate fires
on a real back/forward. That made every genuine back navigation look
like a replay and get ignored.
Fix: compare against `_lastPathnameAndSearch` (the URL the router
last actively navigated to). After a real back, that tracker still
points at the entry we were on, so a genuine navigation differs and
passes the filter — matching Next.js's `state.as === this.asPath`
semantics.
Verified locally:
- tests/pages-router-i18n-sticky-locale.test.ts: 7/7 pass
- tests/shims.test.ts: 944/944 pass
- tests/e2e/pages-router/{navigation,before-pop-state,hydration,router-events}.spec.ts:
28/28 pass (these were the CI failures from the previous 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.
Clean, well-scoped PR. The three behavioral gaps (empty history state, missing stale-popstate filter, popstate locale from window global instead of history entry) are correctly identified and fixed. The Next.js parity references are solid, and the test coverage is thorough.
A few notes on things I checked:
History state shape — updateHistory() now writes { url, as, options, __N: true, key } which matches Router.changeState in Next.js's router.ts around L1916. The key handling is correct: new key on push, preserved from previousState on replace (with fallback mint if there's no prior state). Good.
saveScrollPosition() minting router state for the initial document entry — This is a subtle but important fix. Without it, the first pushState (from performNavigation) calls saveScrollPosition which replaces the initial null state entry. When the user later navigates back, popstate fires with null/{} state and the foreign-state filter would either misclassify it or the Safari-replay filter couldn't operate. Minting __N: true on the initial entry closes this gap.
Popstate stale-state filter — The isFirstPopStateEvent + isNextRouterState guard correctly mirrors Next.js's onPopState (around L920-999). The comparison withBasePathPrefix(state.as, __basePath) === _lastPathnameAndSearch is the right approach — state.as is app-relative (no basePath), and _lastPathnameAndSearch includes basePath, so recomposing before comparison is correct.
Foreign state handling — The null/undefined passthrough preserving legacy behavior while only filtering out object-state-without-__N is a pragmatic choice that avoids breaking existing popstate tests. Matches the documented Next.js if (!state.__N) return early-exit.
Popstate locale recovery — effectiveLocale = stateLocale ?? window.__VINEXT_LOCALE__ correctly prefers the history entry's locale over the window global, which is the core fix for back/forward across locale changes.
Test relaxation in shims.test.ts — The 5 assertions changed from {} to expect.objectContaining({ __N: true }) are exactly right. They were testing URL normalization / locale-qualified fetch behavior, not history state shape. The relaxed assertions stay focused on their actual concern.
New test file — Good coverage across the three behavioral areas. The installRuntime helper properly exercises the real popstate handler path through installPagesRouterRuntime(). The test for "first popstate with different locale is processed" correctly simulates the URL having already changed (as browsers do before firing popstate).
One minor observation for future work: _isFirstPopStateEvent and _historyKeyCounter are module-level mutable state that doesn't reset between vi.resetModules() calls within the same process. This is fine for the current test structure (each test re-imports the module to get a fresh instance), but worth noting if tests ever share a module instance across multiple popstate scenarios in sequence.
Ship it.
|
Review posted. Approved PR #1407 with a detailed review covering:
|
Closes the remaining gap in the popstate locale-recovery path: the initial document entry has `state: null` until the first push, so a back-navigation to it would fall back to the live `window.__VINEXT_LOCALE__` — which an intervening locale-changing push may have flipped. Stamping router-shaped state at install captures the SSR-resolved locale into `options.locale` and lets the popstate handler restore the correct locale. Also: - Strip the hash from `state.as` so the Safari-replay filter's `withBasePath(state.as) === _lastPathnameAndSearch` comparison works for entries created from a hash-bearing URL (the tracker never carries a hash). - Drop the unnecessary `withBasePath as withBasePathPrefix` import alias. Adds 3 tests: install-time stamping, no-clobber of pre-existing state, and the back-to-initial-entry locale-recovery regression.
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-structured PR. The three behavioral gaps are correctly identified and the fixes match the Next.js reference implementation. I verified the key parity points against .nextjs-ref/packages/next/src/shared/lib/router/router.ts:
History state shape — updateHistory() now writes { url, as, options, __N: true, key }, matching Router.changeState (L1916–1929). Key handling is correct: Next.js does method !== 'pushState' ? this._key : createKey() and vinext mirrors this with mode === "push" ? createHistoryKey() : (previousState?.key ?? createHistoryKey()). The fallback createHistoryKey() on replace when there's no previous key is a pragmatic addition since vinext doesn't have a persistent this._key instance field — correct behavior.
navState.as semantics — state.as is set to resolvedNoHash (app-relative, no basePath, no hash), and the popstate comparison correctly recomposes basePath via withBasePath(state.as, __basePath) === _lastPathnameAndSearch. This is the right approach since _lastPathnameAndSearch tracks window.location.pathname + window.location.search (includes basePath, excludes hash). In Next.js, the comparison is state.as === this.asPath where both sides are basePath-free — vinext gets the same result by adding basePath to both sides. Good.
Popstate stale-state filter — The isFirstPopStateEvent + isNextRouterState guard faithfully mirrors Next.js's onPopState (L900–942). The filter structure is:
!state→ Next.js doesreplaceStateand returns; vinext falls through to legacy behavior (null/undefined passthrough). This is a deliberate compat choice documented in the comment — fine.!state.__N→ both return early.- First + matching locale + matching as → both return early.
One subtle difference: Next.js also checks state.__NA (app-router history entries) and does window.location.reload(). vinext doesn't have this because it's pages-router-only code and __NA entries can't exist. Correct omission.
Foreign state handling — The null/undefined passthrough preserving legacy behavior is the right call. Next.js's if (!state) branch does a replaceState + return which means null-state popstates don't trigger navigation either. vinext's approach of allowing null through to the existing hash-only / full-navigation path is slightly different but behaviorally equivalent for the common cases (initial entry, hash changes). The explicit carve-out for object-state-without-__N is the important fix.
Initial entry stamping — stampInitialHistoryState() in installPagesRouterRuntime() is a smart addition. The guard if (window.history.state !== null && window.history.state !== undefined) return prevents clobbering pre-existing state (e.g., server-rendered state or third-party code). The buildInitialRouterState() fallback in saveScrollPosition() is a good defensive measure for the edge case where Router.push fires before installPagesRouterRuntime().
Popstate locale recovery — effectiveLocale = stateLocale ?? window.__VINEXT_LOCALE__ correctly prefers the history entry's locale. This is the core fix for the "back across locale switch fetches wrong URL" bug.
Test changes — The 5 relaxed assertions in shims.test.ts are well-scoped. The new test file covers all three behavioral areas with good scenario design, especially the "back-nav to initial entry" test which exercises the full locale-switch-then-back flow.
One minor observation (not blocking): For hash-only navigations (line 1012), navState.as ends up as stripHash(resolved) which for a #foo input becomes "". This is harmless since hash-only popstates are detected before the stale-state filter runs, but it means the as field in history state for hash-only entries is semantically empty. If this ever matters for debugging, the initial entry stamp already has the correct as, and the hash-only entry just adds noise to the history stack. No action needed.
Ship it.
|
Review posted. Approved PR #1407 with a detailed review covering:
|
…ticky-locale # Conflicts: # packages/vinext/src/shims/router.ts
1 participant
Refs #1336 (item 2).
Summary
Locale stickiness for the Pages Router client runtime. Three behaviours that previously diverged from Next.js:
History state was empty (
{}). Next.js writes{ url, as, options, __N: true, key }on everypushState/replaceState(seeRouter.changeStatearound L1916). Without this shape, popstate handlers can't tell our entries apart from third-party history writes and can't recover the active locale on back/forward.Popstate ignored stale state filtering. Next.js drops a popstate event when it's the first one observed and its
state.options.locale+state.asmatch the current page (Safari tab-restore / BFCache replay). vinext's handler always re-fetched.Popstate re-detected locale from
window.__VINEXT_LOCALE__rather than the locale recorded in the history entry, so back/forward across locale changes used the wrong fetch URL.This PR:
updateHistory()inpackages/vinext/src/shims/router.tsto write the Next.js-shaped state, populated with the active locale resolved via the existingresolveTransitionLocalehelper.isFirstPopStateEventflag and anisNextRouterStateguard in the popstate handler. Foreign state (no__N: true) is ignored. A first-event replay with matching locale+asis ignored.state.options.localeis preferred over the window global when computing the popstate fetch URL.keyfield through replaces (preserving the prior key) and mints a new key on each push.Next.js parity references
Local clone at
.nextjs-ref:packages/next/src/shared/lib/router/router.ts—onPopState(L920–999) andRouter.changeState(L1900–1931).test/e2e/ignore-invalid-popstateevent/with-i18n.test.tsandwithout-i18n.test.ts— exercise the stale-state filter, including the "Don't ignore event with different locale" branch.test/e2e/i18n-preferred-locale-detection/i18n-preferred-locale-detection.test.ts— covers the client-side stickiness behaviour we're trying to match (Link to/from a non-locale-prefixed page must not flip the locale to the browser preference).Tests added
tests/pages-router-i18n-sticky-locale.test.ts— 7 tests across three describe blocks:Router.pushwrites{ __N: true, options: { locale } }and that the locale slot is populated from the active locale, explicitlocale: 'fr', ordefaultLocalewhenlocale: false.asis ignored; first popstate with a different locale is processed; foreign state (no__N) is ignored.Router.pushwith no explicit locale carries the current locale in history state.I also relaxed 5 existing assertions in
tests/shims.test.ts(in the "Pages Router router helpers" and "Pages Router concurrent navigation" blocks) that pinnedpushStateto{}— they now useexpect.objectContaining({ __N: true })so the assertions stay focused on what they're actually testing (URL normalisation / locale-qualified fetch) without depending on the empty-state shape.Scope boundaries with parallel PRs
packages/vinext/src/server/api*or route matching.packages/vinext/src/config/config-matchers.ts,pages-i18n.ts,dev-server.ts, andprod-server.tsare unchanged.parseCookieLocaleFromHeaderalready gives the cookie priority over Accept-Language. Whether vinext should automaticallySet-Cookie: NEXT_LOCALEon initial detection is a separate question — Next.js itself doesn't do this (cookie is user-set, e.g. by a language switcher), and moving it would overlap with item 4's redirect work. Documented as a follow-up.Verification