Skip to content
111 changes: 96 additions & 15 deletions packages/vinext/src/shims/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,6 @@ interface TransitionOptions {
locale?: string;
}

// Route event handler types (used by consumers via router.events)
type _RouteChangeHandler = (url: string) => void;
type _RouteErrorHandler = (err: Error, url: string) => void;

interface RouterEvents {
on(event: string, handler: (...args: unknown[]) => void): void;
off(event: string, handler: (...args: unknown[]) => void): void;
Expand Down Expand Up @@ -156,6 +152,20 @@ export function isExternalUrl(url: string): boolean {
return /^[a-z][a-z0-9+.-]*:/i.test(url) || url.startsWith("//");
}

/** Resolve a hash URL to a basePath-stripped app URL for event payloads */
function resolveHashUrl(url: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (typeof window === "undefined") return url;
if (url.startsWith("#"))
return stripBasePath(window.location.pathname, __basePath) + window.location.search + url;
// Full-path hash URL — strip basePath for consistency with other events
try {
const parsed = new URL(url, window.location.href);
return stripBasePath(parsed.pathname, __basePath) + parsed.search + parsed.hash;
} catch {
return url;
}
}

/** Check if a href is only a hash change relative to the current URL */
export function isHashOnlyChange(href: string): boolean {
if (href.startsWith("#")) return true;
Expand Down Expand Up @@ -420,7 +430,7 @@ async function navigateClient(url: string): Promise<void> {
root.render(element);
} catch (err) {
console.error("[vinext] Client navigation failed:", err);
routerEvents.emit("routeChangeError", err, url);
routerEvents.emit("routeChangeError", err, url, { shallow: false });
window.location.href = url;
} finally {
_navInProgress = false;
Expand Down Expand Up @@ -505,22 +515,32 @@ export function useRouter(): NextRouter {

// Hash-only change — no page fetch needed
if (isHashOnlyChange(resolved)) {
const eventUrl = resolveHashUrl(resolved);
routerEvents.emit("hashChangeStart", eventUrl, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: 6 near-identical hash event blocks.

The hash-change event emission pattern (compute eventUrl, emit start, do work, emit complete) is repeated 4 times for useRouter().push, useRouter().replace, Router.push, and Router.replace, plus a variation in popstate. Each block is ~12 lines with only pushState vs replaceState and setState presence differing.

Not asking you to refactor in this PR — the duplication is pre-existing and the patterns are clear. But a future follow-up could extract the hash-change logic into a shared helper to reduce the surface area for bugs when the event contract changes again.

shallow: options?.shallow ?? false,
});
const hash = resolved.includes("#") ? resolved.slice(resolved.indexOf("#")) : "";
window.history.pushState({}, "", resolved.startsWith("#") ? resolved : full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
scrollToHash(hash);
setState(getPathnameAndQuery());
routerEvents.emit("hashChangeComplete", eventUrl, {
shallow: options?.shallow ?? false,
});
window.dispatchEvent(new CustomEvent("vinext:navigate"));
return true;
}

saveScrollPosition();
routerEvents.emit("routeChangeStart", resolved);
routerEvents.emit("routeChangeStart", resolved, { shallow: options?.shallow ?? false });
routerEvents.emit("beforeHistoryChange", resolved, { shallow: options?.shallow ?? false });
window.history.pushState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
}
setState(getPathnameAndQuery());
routerEvents.emit("routeChangeComplete", resolved);
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });

// Scroll: handle hash target, else scroll to top unless scroll:false
const hash = resolved.includes("#") ? resolved.slice(resolved.indexOf("#")) : "";
Expand Down Expand Up @@ -553,21 +573,31 @@ export function useRouter(): NextRouter {

// Hash-only change — no page fetch needed
if (isHashOnlyChange(resolved)) {
const eventUrl = resolveHashUrl(resolved);
routerEvents.emit("hashChangeStart", eventUrl, {
shallow: options?.shallow ?? false,
});
const hash = resolved.includes("#") ? resolved.slice(resolved.indexOf("#")) : "";
window.history.replaceState({}, "", resolved.startsWith("#") ? resolved : full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
scrollToHash(hash);
setState(getPathnameAndQuery());
routerEvents.emit("hashChangeComplete", eventUrl, {
shallow: options?.shallow ?? false,
});
window.dispatchEvent(new CustomEvent("vinext:navigate"));
return true;
}

routerEvents.emit("routeChangeStart", resolved);
routerEvents.emit("routeChangeStart", resolved, { shallow: options?.shallow ?? false });
routerEvents.emit("beforeHistoryChange", resolved, { shallow: options?.shallow ?? false });
window.history.replaceState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
}
setState(getPathnameAndQuery());
routerEvents.emit("routeChangeComplete", resolved);
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });

// Scroll: handle hash target, else scroll to top unless scroll:false
const hash = resolved.includes("#") ? resolved.slice(resolved.indexOf("#")) : "";
Expand Down Expand Up @@ -623,6 +653,12 @@ export function useRouter(): NextRouter {
// If it returns false, the navigation is cancelled.
let _beforePopStateCb: BeforePopStateCallback | undefined;

// Track pathname+search for detecting hash-only back/forward in the popstate
// handler. Updated after every pushState/replaceState so that popstate can
// compare the previous value with the (already-changed) window.location.
let _lastPathnameAndSearch =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good design. The _lastPathnameAndSearch tracking is the right approach for detecting hash-only popstate. The initialization at module load time is correct: even if the user lands on /page#section, the initial value is /page (pathname + search only), so the first popstate after a hash push correctly detects isHashOnly = true.

typeof window !== "undefined" ? window.location.pathname + window.location.search : "";

// Module-level popstate listener: handles browser back/forward by re-rendering
// the React root with the page at the new URL. This runs regardless of whether
// any component calls useRouter().
Expand All @@ -631,6 +667,9 @@ if (typeof window !== "undefined") {
const browserUrl = window.location.pathname + window.location.search;
const appUrl = stripBasePath(window.location.pathname, __basePath) + window.location.search;

// Detect hash-only back/forward: pathname+search unchanged, only hash differs.
Comment on lines 667 to +670
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good design choice using pathname+search (excluding hash) for the comparison. This correctly identifies hash-only navigation: if only the hash changed, browserUrl (which is pathname + search) will match _lastPathnameAndSearch.

Edge case that works correctly: if the user lands directly on /page#section, _lastPathnameAndSearch is initialized to /page (line 668). A subsequent back-button press to a different page will have a different browserUrl, so isHashOnly will be false — correct behavior.

const isHashOnly = browserUrl === _lastPathnameAndSearch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming I've thought through the edge cases for _lastPathnameAndSearch tracking — looks solid.

If the user lands on /page#section directly, _lastPathnameAndSearch initializes to /page. A subsequent hash-only push and back button will correctly detect isHashOnly = true since browserUrl (/page) matches. The non-hash case (navigating to a different page then back) correctly gets isHashOnly = false because browserUrl differs.

One subtlety: if the user lands on /page#section and the popstate handler fires for the initial page (some browsers do this on page load), isHashOnly would be true and hash events would fire spuriously. But modern browsers (Chrome 34+, Firefox, Safari) no longer fire popstate on initial page load, so this is fine.


// Check beforePopState callback
if (_beforePopStateCb !== undefined) {
const shouldContinue = (_beforePopStateCb as BeforePopStateCallback)({
Expand All @@ -641,9 +680,31 @@ if (typeof window !== "undefined") {
if (!shouldContinue) return;
}

routerEvents.emit("routeChangeStart", appUrl);
// Update tracker only after beforePopState confirms navigation proceeds.
// If beforePopState cancels, the tracker must retain the previous value
// so the next popstate compares against the correct baseline.
_lastPathnameAndSearch = browserUrl;

if (isHashOnly) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add _lastPathnameAndSearch = browserUrl; here, just before this if block — this is where the line removed from 676 should go:

Suggested change
if (isHashOnly) {
_lastPathnameAndSearch = browserUrl;
if (isHashOnly) {

// Hash-only back/forward — no page fetch needed
const hashUrl = appUrl + window.location.hash;
routerEvents.emit("hashChangeStart", hashUrl, { shallow: false });
Comment on lines +688 to +691
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hash-only popstate branch is a good addition — avoids the unnecessary navigateClient() fetch that would otherwise happen on hash-only back/forward.

Note for a follow-up: this branch doesn't trigger a React state update, so components reading router.asPath via useRouter() won't reflect the hash change from browser back/forward. The non-hash path implicitly handles this via navigateClient() which re-renders the tree. Consider adding a // TODO: trigger React re-render for hash-only popstate comment here so it's not lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually already handled — line 698 dispatches vinext:navigate, which the useRouter() hook listens for (line 498) and calls setState(getPathnameAndQuery()). Since getPathnameAndQuery() reads window.location.hash into asPath (line 321), components reading router.asPath will re-render with the updated hash after hash-only popstate.

scrollToHash(window.location.hash);
routerEvents.emit("hashChangeComplete", hashUrl, { shallow: false });
window.dispatchEvent(new CustomEvent("vinext:navigate"));
return;
}

const fullAppUrl = appUrl + window.location.hash;
routerEvents.emit("routeChangeStart", fullAppUrl, { shallow: false });
// Note: The browser has already updated window.location by the time popstate
// fires, so this is not truly "before" the URL change. In Next.js the popstate
// handler calls replaceState to store history metadata — beforeHistoryChange
// precedes that call, not the URL change itself. We emit it here for API
// compatibility.
routerEvents.emit("beforeHistoryChange", fullAppUrl, { shallow: false });
void navigateClient(browserUrl).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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("routeChangeComplete", appUrl);
routerEvents.emit("routeChangeComplete", fullAppUrl, { shallow: false });
restoreScrollPosition(e.state);
window.dispatchEvent(new CustomEvent("vinext:navigate"));
});
Expand Down Expand Up @@ -693,20 +754,30 @@ const Router = {

// Hash-only change
if (isHashOnlyChange(resolved)) {
const eventUrl = resolveHashUrl(resolved);
routerEvents.emit("hashChangeStart", eventUrl, {
shallow: options?.shallow ?? false,
});
const hash = resolved.includes("#") ? resolved.slice(resolved.indexOf("#")) : "";
window.history.pushState({}, "", resolved.startsWith("#") ? resolved : full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
scrollToHash(hash);
routerEvents.emit("hashChangeComplete", eventUrl, {
Comment on lines +757 to +765
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing issue surfaced by this PR: The Router singleton's hash-only paths emit events correctly but don't call setState(getPathnameAndQuery()) (which the useRouter() hook's equivalent paths do at lines 530 and 591).

This means Router.push('#section') will fire hashChangeStart/hashChangeComplete events, but a component reading router.asPath won't see the hash update until it re-renders for some other reason.

setState is local to useRouter() so it can't be called from the singleton — this is a pre-existing architectural limitation. But now that the singleton has full event parity, it's worth noting as a known gap.

shallow: options?.shallow ?? false,
});
window.dispatchEvent(new CustomEvent("vinext:navigate"));
return true;
}

saveScrollPosition();
routerEvents.emit("routeChangeStart", resolved);
routerEvents.emit("routeChangeStart", resolved, { shallow: options?.shallow ?? false });
routerEvents.emit("beforeHistoryChange", resolved, { shallow: options?.shallow ?? false });
window.history.pushState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
}
routerEvents.emit("routeChangeComplete", resolved);
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });

const hash = resolved.includes("#") ? resolved.slice(resolved.indexOf("#")) : "";
if (hash) {
Expand Down Expand Up @@ -734,19 +805,29 @@ const Router = {

// Hash-only change
if (isHashOnlyChange(resolved)) {
const eventUrl = resolveHashUrl(resolved);
routerEvents.emit("hashChangeStart", eventUrl, {
shallow: options?.shallow ?? false,
});
const hash = resolved.includes("#") ? resolved.slice(resolved.indexOf("#")) : "";
window.history.replaceState({}, "", resolved.startsWith("#") ? resolved : full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
scrollToHash(hash);
routerEvents.emit("hashChangeComplete", eventUrl, {
shallow: options?.shallow ?? false,
});
window.dispatchEvent(new CustomEvent("vinext:navigate"));
return true;
}

routerEvents.emit("routeChangeStart", resolved);
routerEvents.emit("routeChangeStart", resolved, { shallow: options?.shallow ?? false });
routerEvents.emit("beforeHistoryChange", resolved, { shallow: options?.shallow ?? false });
window.history.replaceState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
}
routerEvents.emit("routeChangeComplete", resolved);
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });

const hash = resolved.includes("#") ? resolved.slice(resolved.indexOf("#")) : "";
if (hash) {
Expand Down
123 changes: 123 additions & 0 deletions tests/e2e/pages-router/router-events.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,129 @@ test.describe("router.events (Pages Router)", () => {
expect(events).toContain("complete:/about");
});

test("beforeHistoryChange fires between routeChangeStart and routeChangeComplete", async ({
page,
}) => {
await page.click('[data-testid="push-about"]');
await expect(page.locator("h1")).toHaveText("About");

const events: string[] = await page.evaluate((key) => {
const raw = sessionStorage.getItem(key);
return raw ? JSON.parse(raw) : [];
}, STORAGE_KEY);

const startIdx = events.findIndex((e) => e === "start:/about");
const beforeIdx = events.findIndex((e) => e === "beforeHistoryChange:/about");
const completeIdx = events.findIndex((e) => e === "complete:/about");
expect(startIdx).toBeGreaterThanOrEqual(0);
expect(beforeIdx).toBeGreaterThan(startIdx);
expect(completeIdx).toBeGreaterThan(beforeIdx);
});

test("hashChangeStart and hashChangeComplete fire on hash-only push with full URL", async ({
page,
}) => {
await page.click('[data-testid="push-hash"]');

const events: string[] = await page.evaluate((key) => {
const raw = sessionStorage.getItem(key);
return raw ? JSON.parse(raw) : [];
}, STORAGE_KEY);

// Event URL should include pathname, not just the fragment
expect(events).toContain("hashChangeStart:/router-events-test#section-1");
expect(events).toContain("hashChangeComplete:/router-events-test#section-1");
// Should NOT fire routeChange or beforeHistoryChange events for hash-only navigation
expect(events.some((e) => e.startsWith("start:"))).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.some((e) => e.startsWith("complete:"))).toBe(false);
expect(events.some((e) => e.startsWith("beforeHistoryChange:"))).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good — the beforeHistoryChange negative assertion was already addressed (matching bonk's suggestion). Tests are thorough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good — the beforeHistoryChange negative assertion was addressed from the prior review. Tests are thorough and cover the important scenarios: push, replace, ordering, back, forward, and negative assertions for event exclusivity.

});

test("hashChangeStart fires before hashChangeComplete on hash push", async ({ page }) => {
await page.click('[data-testid="push-hash"]');

const events: string[] = await page.evaluate((key) => {
const raw = sessionStorage.getItem(key);
return raw ? JSON.parse(raw) : [];
}, STORAGE_KEY);

const startIdx = events.findIndex((e) => e.startsWith("hashChangeStart:"));
const completeIdx = events.findIndex((e) => e.startsWith("hashChangeComplete:"));
expect(startIdx).toBeGreaterThanOrEqual(0);
expect(completeIdx).toBeGreaterThan(startIdx);
});

test("hashChangeStart and hashChangeComplete fire on hash-only replace with full URL", async ({
page,
}) => {
await page.click('[data-testid="replace-hash"]');

const events: string[] = await page.evaluate((key) => {
const raw = sessionStorage.getItem(key);
return raw ? JSON.parse(raw) : [];
}, STORAGE_KEY);

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here — add a beforeHistoryChange negative assertion for the replace-hash test:

Suggested change
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);

expect(events.some((e) => e.startsWith("complete:"))).toBe(false);
expect(events.some((e) => e.startsWith("beforeHistoryChange:"))).toBe(false);
});

test("hash-only back/forward emits hashChange events, not routeChange", async ({ page }) => {
// Push a hash change, then go back — popstate should detect hash-only navigation
await page.click('[data-testid="push-hash"]');
await page.click('[data-testid="clear-events"]');
await page.goBack();

// Wait for popstate handler to record events
await page.waitForFunction((key) => {
const raw = sessionStorage.getItem(key);
return raw ? JSON.parse(raw).length > 0 : false;
}, STORAGE_KEY);

const events: string[] = await page.evaluate((key) => {
const raw = sessionStorage.getItem(key);
return raw ? JSON.parse(raw) : [];
}, STORAGE_KEY);

// Should emit hashChange events for hash-only back navigation
expect(events.some((e) => e.startsWith("hashChangeStart:"))).toBe(true);
expect(events.some((e) => e.startsWith("hashChangeComplete:"))).toBe(true);
// Should NOT emit routeChange or beforeHistoryChange events
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);
});
Comment on lines +126 to +150
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent test — this exercises the _lastPathnameAndSearch popstate detection end-to-end. The clear-events click between the hash push and goBack() is a nice touch to isolate the popstate events.

Consider adding a goForward() test in a follow-up to verify hash-only detection works in both directions.


test("hash-only forward emits hashChange events, not routeChange", async ({ page }) => {
// Push a hash, go back, wait for popstate to settle, clear events, then go forward
await page.click('[data-testid="push-hash"]');
await page.goBack();
await page.waitForFunction((key) => {
const raw = sessionStorage.getItem(key);
return raw ? JSON.parse(raw).length > 0 : false;
}, STORAGE_KEY);
await page.click('[data-testid="clear-events"]');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: potential flakiness in forward test.

goBack() triggers a popstate event whose handler writes to sessionStorage (the hash events from the back navigation). The clear-events click on the next line should run after popstate completes because the hash-only popstate handler is synchronous and Playwright awaits the navigation. But if this test ever becomes flaky, adding a waitForFunction between goBack() and clear-events (like you did in the back test at lines 132-136) would make the sequencing explicit.

Not a required change — just a note for debugging if it ever flakes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor robustness note: after goBack() on line 155, the waitForFunction waits for events from the back navigation. Then clear-events on this line runs immediately. Since the hash-only popstate handler is synchronous and the clear-events handler directly calls sessionStorage.removeItem() (not dependent on React state), and the subsequent waitForFunction checks sessionStorage rather than the DOM, this should be robust. Just noting as a debugging hint if this ever becomes intermittent — adding a waitForFunction between goBack() settlement and clear-events (like the back test at lines 132-136) would make the sequencing fully explicit.

await page.goForward();

await page.waitForFunction((key) => {
const raw = sessionStorage.getItem(key);
return raw ? JSON.parse(raw).length > 0 : false;
}, STORAGE_KEY);

const events: string[] = await page.evaluate((key) => {
const raw = sessionStorage.getItem(key);
return raw ? JSON.parse(raw) : [];
}, STORAGE_KEY);

expect(events.some((e) => e.startsWith("hashChangeStart:"))).toBe(true);
expect(events.some((e) => e.startsWith("hashChangeComplete:"))).toBe(true);
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);
});

test("multiple navigations produce multiple event pairs", async ({ page }) => {
// Navigate to about
await page.click('[data-testid="push-about"]');
Expand Down
Loading
Loading