fix: emit hashChangeStart/Complete and beforeHistoryChange router events#468
Conversation
Next.js emits hashChangeStart/hashChangeComplete for hash-only
navigation and beforeHistoryChange before every history.pushState/
replaceState call. Our router emitted neither.
- Hash-only push/replace now emit hashChangeStart before and
hashChangeComplete after (instead of routeChange* events)
- Normal push/replace now emit beforeHistoryChange between
routeChangeStart and the pushState/replaceState call
- All events now pass { shallow } as the second argument per Next.js
- Popstate handler also emits beforeHistoryChange
- Updated in both useRouter() hook and Router singleton code paths
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: d7cbb79d51
ℹ️ 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".
packages/vinext/src/shims/router.ts
Outdated
|
|
||
| // Hash-only change — no page fetch needed | ||
| if (isHashOnlyChange(resolved)) { | ||
| routerEvents.emit("hashChangeStart", resolved, { shallow: options?.shallow ?? false }); |
There was a problem hiding this comment.
When callers do router.push('#section'), this emits hashChangeStart/hashChangeComplete with resolved ('#section') even though the browser URL becomes the current path plus hash (via history.pushState). Consumers that rely on Next.js-compatible event payloads cannot tell which route changed and will mis-handle hash events on shared listeners; this is especially visible when multiple pages use hash navigation. Emit the same resolved browser URL you write to history (including pathname/search and basePath) instead of the fragment-only string.
Useful? React with 👍 / 👎.
When router.push("#section") is called, emit the full resolved URL
(e.g., "/page#section") in hashChangeStart/hashChangeComplete events
instead of just "#section". Matches Next.js behavior where the event
URL includes the pathname.
|
/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: Router hash & beforeHistoryChange events
Good contribution — this fills real gaps in the Pages Router event contract. The beforeHistoryChange placement, the { shallow } second argument, and the hash event pairs all match Next.js behavior. Tests are well-structured and cover the important scenarios.
A few issues to address:
| Severity | Issue |
|---|---|
| Bug | Popstate handler emits beforeHistoryChange but the browser has already changed the URL before popstate fires |
| Bug | Popstate handler doesn't distinguish hash-only back/forward — should emit hashChangeStart/hashChangeComplete instead of routeChangeStart/routeChangeComplete when navigating back from a hash change |
| Nit | resolveHashUrl is called twice per hash navigation — could cache the result |
| Nit | Tests should assert beforeHistoryChange does NOT fire for hash-only navigation |
| routerEvents.emit("routeChangeStart", appUrl); | ||
| routerEvents.emit("routeChangeStart", appUrl, { shallow: false }); | ||
| routerEvents.emit("beforeHistoryChange", appUrl, { shallow: false }); | ||
| void navigateClient(browserUrl).then(() => { |
There was a problem hiding this comment.
Bug: beforeHistoryChange in the popstate handler is semantically misleading.
For popstate (browser back/forward), the browser has already changed window.location by the time the popstate event fires. So beforeHistoryChange here fires after the URL has changed, not before.
In Next.js, the popstate handler calls this.change('replaceState', ...) which emits beforeHistoryChange before calling this.changeState() — but that's before a replaceState call that stores Next.js metadata into history state, not before the actual URL change (which already happened).
This is arguably acceptable as a simplification since vinext doesn't store history state metadata. But if you keep it, a comment explaining the difference would help future readers understand this isn't truly "before" the history change.
| } | ||
|
|
||
| routerEvents.emit("routeChangeStart", appUrl); | ||
| routerEvents.emit("routeChangeStart", appUrl, { shallow: false }); |
There was a problem hiding this comment.
Bug: Popstate handler doesn't handle hash-only back/forward navigation.
When a user does router.push('#section-1') then hits the browser back button, the popstate handler fires. The URL change is hash-only, but this code unconditionally emits routeChangeStart/routeChangeComplete and calls navigateClient() (which fetches the page HTML again unnecessarily).
In Next.js, the popstate handler flows through this.change() which checks onlyAHashChange() and branches to emit hashChangeStart/hashChangeComplete instead.
Since this PR is specifically about hash events, it would be good to handle this case too. A simple approach: since appUrl is pathname + search (no hash), you can track the previous pathname + search and compare — if they match but location.hash differs, treat it as hash-only and emit hashChangeStart/hashChangeComplete instead of calling navigateClient().
This would also avoid the unnecessary network fetch on hash-only back/forward.
packages/vinext/src/shims/router.ts
Outdated
|
|
||
| // Hash-only change — no page fetch needed | ||
| if (isHashOnlyChange(resolved)) { | ||
| routerEvents.emit("hashChangeStart", resolveHashUrl(resolved), { |
There was a problem hiding this comment.
Nit: resolveHashUrl(resolved) is computed identically on both lines 518 and 529 (and similarly in every other hash code path). Since the window state doesn't change between them (the hash events are synchronous), consider computing it once:
const eventUrl = resolveHashUrl(resolved);
routerEvents.emit("hashChangeStart", eventUrl, { shallow: ... });
// ...
routerEvents.emit("hashChangeComplete", eventUrl, { shallow: ... });Applies to all 4 hash code paths.
| } | ||
|
|
||
| /** Resolve a fragment-only URL to include the current pathname + search */ | ||
| function resolveHashUrl(url: string): string { |
There was a problem hiding this comment.
The function is clean and correct. Handles both cases well: fragment-only URLs (#foo) get the current pathname prepended, and full-path URLs (/page#hash) pass through unchanged since isHashOnlyChange can return true for both forms.
| expect(events).toContain("hashChangeStart:/router-events-test#section-1"); | ||
| expect(events).toContain("hashChangeComplete:/router-events-test#section-1"); | ||
| // Should NOT fire routeChange events for hash-only navigation | ||
| expect(events.some((e) => e.startsWith("start:"))).toBe(false); |
There was a problem hiding this comment.
Good negative assertions. Consider also asserting that beforeHistoryChange does NOT fire for hash-only navigation — in Next.js, the hash change code path is a completely separate branch that returns early before reaching the beforeHistoryChange emit:
| expect(events.some((e) => e.startsWith("start:"))).toBe(false); | |
| // Should NOT fire routeChange events for hash-only navigation | |
| expect(events.some((e) => e.startsWith("start:"))).toBe(false); | |
| expect(events.some((e) => e.startsWith("complete:"))).toBe(false); | |
| expect(events.some((e) => e.startsWith("beforeHistoryChange:"))).toBe(false); |
|
|
||
| expect(events).toContain("hashChangeStart:/router-events-test#section-2"); | ||
| expect(events).toContain("hashChangeComplete:/router-events-test#section-2"); | ||
| expect(events.some((e) => e.startsWith("start:"))).toBe(false); |
There was a problem hiding this comment.
Same here — add a beforeHistoryChange negative assertion for the replace-hash test:
| expect(events.some((e) => e.startsWith("start:"))).toBe(false); | |
| expect(events.some((e) => e.startsWith("start:"))).toBe(false); | |
| expect(events.some((e) => e.startsWith("complete:"))).toBe(false); | |
| expect(events.some((e) => e.startsWith("beforeHistoryChange:"))).toBe(false); |
|
Review posted successfully. Here's a summary of the feedback I left on PR #468: Review posted on PR #468 with 6 inline comments covering:
|
|
@james-elicx Claude is down bro I'll finish this when I wake up tomorrow |
|
No worries :D |
- Detect hash-only back/forward in popstate handler by tracking pathname+search across navigations; emit hashChangeStart/Complete instead of routeChangeStart/Complete and skip unnecessary page fetch - Add comment explaining beforeHistoryChange semantics in popstate (fires after URL change, emitted for API compatibility) - Cache resolveHashUrl() result in all 4 hash code paths - Add beforeHistoryChange negative assertions in hash-only tests - Add E2E test for hash-only back/forward navigation events
2 participants
Summary
router.push("#section")) now emitshashChangeStartbefore andhashChangeCompleteafter, matching Next.js behavior. Previously emitted zero events.beforeHistoryChangenow fires betweenrouteChangeStartandhistory.pushState/replaceStateon normal navigation. Previously never emitted.{ shallow }as the second argument, matching the Next.js contract.useRouter().push,useRouter().replace,Router.push,Router.replace, and the popstate handler.Ported from Next.js event contract documented in:
docs/02-pages/04-api-reference/03-functions/use-router.mdxTest plan
beforeHistoryChangefires betweenrouteChangeStartandrouteChangeCompleteonrouter.push()hashChangeStartandhashChangeCompletefire on hash-onlyrouter.push("#section")hashChangeStartfires beforehashChangeCompletehashChangeStart/Completefire onrouter.replace("#section")routeChangeStart/routeChangeComplete