Skip to content
176 changes: 151 additions & 25 deletions packages/vinext/src/shims/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,11 +340,42 @@ function getPathnameAndQuery(): {
return { pathname, query, asPath };
}

/**
* Error thrown when a navigation is superseded by a newer one.
* Matches Next.js's convention of an Error with `.cancelled = true`.
*/
class NavigationCancelledError extends Error {
cancelled = true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: consider exporting NavigationCancelledError (or at least the type) so consumers can distinguish cancellations from real errors in routeChangeError listeners without relying on duck-typing .cancelled. Next.js doesn't export a named class for this, so duck-typing is the documented pattern — but exporting the class would be a nice DX improvement.

Not blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed this would be nice DX. Keeping duck-typing for now to match Next.js's convention — consumers already check .cancelled. Can revisit as a follow-up if there's demand.

constructor(route: string) {
super(`Abort fetching component for route: "${route}"`);
this.name = "NavigationCancelledError";
}
}

/**
* Monotonically increasing ID for tracking the current navigation.
* Each call to navigateClient() increments this and captures the value.
* After each async boundary, the navigation checks whether it is still
* the active one. If a newer navigation has started, the stale one
* throws NavigationCancelledError so the caller can emit routeChangeError
* and skip routeChangeComplete.
*
* Replaces the old boolean `_navInProgress` guard which silently dropped
* the second navigation, causing URL/content mismatch.
*/
let _navigationId = 0;

/** AbortController for the in-flight fetch, so superseded navigations abort network I/O. */
let _activeAbortController: AbortController | null = null;

/**
* Perform client-side navigation: fetch the target page's HTML,
* extract __NEXT_DATA__, and re-render the React root.
*
* Throws NavigationCancelledError if a newer navigation supersedes this one.
* Throws on hard-navigation failures (non-OK response, missing data) so the
* caller can distinguish success from failure for event emission.
*/
let _navInProgress = false;
async function navigateClient(url: string): Promise<void> {
if (typeof window === "undefined") return;

Expand All @@ -355,30 +386,65 @@ async function navigateClient(url: string): Promise<void> {
return;
}

// Prevent re-entrant navigation (e.g., double popstate events)
if (_navInProgress) return;
_navInProgress = true;
// Cancel any in-flight navigation (abort its fetch, mark it stale)
_activeAbortController?.abort();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good: aborting the previous controller before creating a new one is the correct order. If the abort handler in the previous navigation's fetch runs synchronously (some environments do this), it will see _navigationId already incremented and correctly throw NavigationCancelledError.

One edge case to consider: if navigateClient is called with the same URL (e.g., user double-clicks a link), the current navigation aborts itself. This is arguably correct (the second call takes over), but it does mean the first fetch is wasted even though it was fetching the same resource. Not a bug, just a minor inefficiency — and it matches Next.js behavior.

const controller = new AbortController();
_activeAbortController = controller;

const navId = ++_navigationId;

/** Check if this navigation is still the active one. If not, throw. */
function assertStillCurrent(): void {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good: the closure captures navId by value, so each navigation has a stable snapshot to compare against. The inner function is also nicely minimal — no allocation, just an integer comparison and throw.

One thought: if you ever need to expose cancellation status to external code (e.g., for testing or debugging), consider returning a boolean from assertStillCurrent() instead of throwing, and let the caller decide whether to throw. But for now the throw pattern is the right call — it's simpler and matches the Next.js clc pattern.

if (navId !== _navigationId) {
throw new NavigationCancelledError(url);
}
}

try {
// Fetch the target page's SSR HTML
const res = await fetch(url, { headers: { Accept: "text/html" } });
let res: Response;
try {
res = await fetch(url, {
headers: { Accept: "text/html" },
signal: controller.signal,
});
} catch (err: unknown) {
// AbortError means a newer navigation cancelled this fetch
if (err instanceof DOMException && err.name === "AbortError") {
throw new NavigationCancelledError(url);
}
throw err;
}
assertStillCurrent();

if (!res.ok) {
// Set window.location.href first so the browser navigates to the correct
// page even if the caller suppresses the error. The assignment schedules
// the navigation asynchronously (as a task), so synchronous routeChangeError
// listeners still run — and observe the error — before the page unloads.
// Contract: routeChangeError listeners MUST be synchronous; async listeners
// will not fire before the navigation completes. Callers (runNavigateClient)
// must NOT schedule a second hard navigation — this assignment already queues one.
window.location.href = url;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good comment explaining the window.location.href + throw interaction. One small addition worth noting: the callers catch this error and return false, but they don't prevent the hard navigation already scheduled by window.location.href = url. So the caller gets false AND a hard navigation happens. This is correct (caller learns it failed, browser recovers), but callers should not attempt their own hard-navigation fallback since one is already in flight.

Worth adding a note like: // Callers must NOT schedule a second hard navigation — this assignment already queues one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a note: Callers (runNavigateClient) must NOT schedule a second hard navigation — this assignment already queues one.

return;
const err = new Error(`Navigation failed: ${res.status} ${res.statusText}`);
throw err;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ordering concern: window.location.href = url triggers a hard navigation, then throw err fires. In a real browser, throw would likely still execute because location.href assignment doesn't synchronously unload the page. But the thrown error propagates to the caller, which emits routeChangeError — is that the intended behavior?

In the old code, these were early returns, so the caller proceeded to emit routeChangeComplete (which was wrong, hence the fix). The new throw correctly prevents routeChangeComplete, but the caller now emits routeChangeError and the page is doing a hard navigation. This double-action seems intentional (the error event lets listeners know what happened before the page unloads), but it means routeChangeError listeners must be synchronous to observe the event before the hard nav triggers. Worth a brief comment explaining this contract.


const html = await res.text();
assertStillCurrent();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug (low-severity): window.__NEXT_DATA__ is written on line 444 (below), between this assertStillCurrent() and the next one on line 472. Everything between lines 433–471 is synchronous so there's no microtask interleaving within this function, but the issue is:

  1. Stale navigation A's fetch resolves → passes assertStillCurrent() here
  2. Synchronously writes window.__NEXT_DATA__ = nextDataA (line 444)
  3. Hits await import(pageModuleUrl) on line 471 — yields to microtask queue
  4. Winning navigation B's microtasks run, eventually overwriting __NEXT_DATA__

Between steps 2 and 4, __NEXT_DATA__ holds stale data. Any code reading it during that window (analytics, routeChangeError listeners from the stale nav's cancellation on line 472) sees inconsistent state.

Fix: Defer window.__NEXT_DATA__ = nextData until just before root.render(element) (around line 516), after all assertStillCurrent() checks have passed. Keep const nextData = JSON.parse(...) and const { pageProps } = nextData.props where they are — just delay the global write.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Deferred the window.__NEXT_DATA__ write to just before root.render(), after all assertStillCurrent() checkpoints have passed. Added a regression test that asserts __NEXT_DATA__ does not reflect the cancelled route's data at the moment routeChangeError fires.


// Extract __NEXT_DATA__ from the HTML
const match = html.match(/<script>window\.__NEXT_DATA__\s*=\s*(.*?)<\/script>/);
if (!match) {
window.location.href = url;
return;
throw new Error("Navigation failed: missing __NEXT_DATA__ in response");
}

const nextData = JSON.parse(match[1]);
const { pageProps } = nextData.props;
window.__NEXT_DATA__ = nextData;
// Defer writing window.__NEXT_DATA__ until just before root.render() —
// writing it here would let a stale navigation briefly pollute the global
// between this assertStillCurrent() and the next one after await import().

// Get the page module URL from __NEXT_DATA__.__vinext (preferred),
// or fall back to parsing the hydration script
Expand All @@ -393,28 +459,31 @@ async function navigateClient(url: string): Promise<void> {

if (!pageModuleUrl) {
window.location.href = url;
return;
throw new Error("Navigation failed: no page module URL found");
}

// Validate the module URL before importing — defense-in-depth against
// unexpected __NEXT_DATA__ or malformed HTML responses
if (!isValidModulePath(pageModuleUrl)) {
console.error("[vinext] Blocked import of invalid page module path:", pageModuleUrl);
window.location.href = url;
return;
throw new Error("Navigation failed: invalid page module path");
}

// Dynamically import the new page module
const pageModule = await import(/* @vite-ignore */ pageModuleUrl);
assertStillCurrent();

const PageComponent = pageModule.default;

if (!PageComponent) {
window.location.href = url;
return;
throw new Error("Navigation failed: page module has no default export");
}

// Import React for createElement
const React = (await import("react")).default;
assertStillCurrent();

// Re-render with the new page, loading _app if needed
let AppComponent = window.__VINEXT_APP__;
Expand All @@ -433,6 +502,7 @@ async function navigateClient(url: string): Promise<void> {
}
}
}
assertStillCurrent();

let element;
if (AppComponent) {
Expand All @@ -447,13 +517,51 @@ async function navigateClient(url: string): Promise<void> {
// Wrap with RouterContext.Provider so next/compat/router works
element = wrapWithRouterContext(element);

// Commit __NEXT_DATA__ only after all assertStillCurrent() checks have passed,
// so a stale navigation can never pollute the global.
window.__NEXT_DATA__ = nextData;
root.render(element);
} catch (err) {
console.error("[vinext] Client navigation failed:", err);
routerEvents.emit("routeChangeError", err, url, { shallow: false });
window.location.href = url;
} finally {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Subtle but important: there is no assertStillCurrent() between the last checkpoint (line 505, after _app import) and this window.__NEXT_DATA__ write + root.render(). The gap covers:

  1. React.createElement(AppComponent, ...) or React.createElement(PageComponent, ...)
  2. wrapWithRouterContext(element)
  3. window.__NEXT_DATA__ = nextData
  4. root.render(element)

All of these are synchronous, so there's no microtask interleaving between them — a newer navigation can't start between lines 507 and 523. This is fine today, but it's a fragile invariant. If wrapWithRouterContext or element construction ever becomes async (e.g., lazy component resolution), the gap would become a real bug.

Worth a brief comment noting the synchronous invariant:

Suggested change
} finally {
// Commit __NEXT_DATA__ only after all assertStillCurrent() checks have passed,
// so a stale navigation can never pollute the global.
// NOTE: Everything from the last assertStillCurrent() (line ~505) through
// root.render() is synchronous — no microtask yield, so no interleaving risk.
window.__NEXT_DATA__ = nextData;

_navInProgress = false;
// Clean up the abort controller if this navigation is still the active one
if (navId === _navigationId) {
_activeAbortController = null;
}
}
Comment on lines 448 to 540
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

navigateClient() no longer has a catch that falls back to hard navigation on unexpected failures. Several operations here can still throw (e.g. JSON.parse(match[1]), dynamic import(pageModuleUrl), import('react'), root.render), but in those cases the caller currently only emits routeChangeError and returns false — leaving the URL already updated via pushState/replaceState/popstate while the old content stays rendered. Consider reintroducing a catch in navigateClient (or adding equivalent fallback in every caller) that, for non-cancel errors, sets window.location.href = url before rethrowing so the browser reliably lands on the correct page.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. runNavigateClient() now does window.location.href = fullUrl for non-cancel errors, restoring the hard-navigation recovery that the old code had.

}

/**
* Run navigateClient and handle errors: emit routeChangeError on failure,
* and fall back to a hard navigation for non-cancel errors so the browser
* recovers to a consistent state.
*
* Returns:
* - "completed" — navigation finished, caller should emit routeChangeComplete
* - "cancelled" — superseded by a newer navigation, caller should return true
* without emitting routeChangeComplete (matches Next.js behaviour)
* - "failed" — genuine error, caller should return false (hard nav is already
* scheduled as recovery)
*/
async function runNavigateClient(
fullUrl: string,
resolvedUrl: string,
): Promise<"completed" | "cancelled" | "failed"> {
try {
await navigateClient(fullUrl);
return "completed";
} catch (err: unknown) {
routerEvents.emit("routeChangeError", err, resolvedUrl, { shallow: false });
if (err instanceof NavigationCancelledError) {
return "cancelled";
}
// Genuine error (network, parse, import failure): fall back to a hard
// navigation so the browser lands on the correct page. navigateClient
// already assigns window.location.href for known failure modes (non-OK
// response, missing __NEXT_DATA__), but unexpected errors (dynamic import
// failure, etc.) would otherwise leave URL and content out of sync.
if (typeof window !== "undefined") {
window.location.href = fullUrl;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug (low severity): This line sets window.location.href = fullUrl for every non-cancel error. But navigateClient() already sets window.location.href = url for all its known failure modes (non-OK response at line 428, missing __NEXT_DATA__ at line 439, no module URL at line 461, invalid path at line 469, no default export at line 480) before throwing.

This means known failures trigger two window.location.href assignments. The second is a no-op in practice, but it contradicts the comment on line 427 which explicitly says callers must NOT schedule a second hard navigation.

The window.location.href fallback here is still valuable for unexpected errors (e.g., JSON.parse throws, import() rejects with a non-abort error) where navigateClient didn't schedule a hard nav. But as written, it fires unconditionally.

Consider guarding it:

Suggested change
window.location.href = fullUrl;
// Genuine error (network, parse, import failure): fall back to a hard
// navigation so the browser lands on the correct page. navigateClient
// already assigns window.location.href for known failure modes (non-OK
// response, missing __NEXT_DATA__), but unexpected errors (dynamic import
// failure, etc.) would otherwise leave URL and content out of sync.
// The duplicate assignment for known failures is a harmless no-op.
if (typeof window !== "undefined") {
window.location.href = fullUrl;
}

Or, longer term, consolidate all hard-nav assignments here and remove them from navigateClient() — let it be a pure "try or throw" function.

}
return "failed";
}
}

Expand Down Expand Up @@ -564,7 +672,9 @@ export function useRouter(): NextRouter {
window.history.pushState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
const result = await runNavigateClient(full, resolved);
if (result === "cancelled") return true;
if (result === "failed") return false;
}
setState(getPathnameAndQuery());
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });
Expand Down Expand Up @@ -621,7 +731,9 @@ export function useRouter(): NextRouter {
window.history.replaceState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
const result = await runNavigateClient(full, resolved);
if (result === "cancelled") return true;
if (result === "failed") return false;
}
setState(getPathnameAndQuery());
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });
Expand Down Expand Up @@ -730,11 +842,21 @@ if (typeof window !== "undefined") {
// 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(() => {
routerEvents.emit("routeChangeComplete", fullAppUrl, { shallow: false });
restoreScrollPosition(e.state);
window.dispatchEvent(new CustomEvent("vinext:navigate"));
});
void navigateClient(browserUrl).then(
() => {
routerEvents.emit("routeChangeComplete", fullAppUrl, { shallow: false });
restoreScrollPosition(e.state);
window.dispatchEvent(new CustomEvent("vinext:navigate"));
},
(err: unknown) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: the if branch and the fallthrough do the exact same thing (routerEvents.emit("routeChangeError", ...)). The instanceof check here doesn't change behavior — both paths emit the same event. If the intent is to differentiate in the future (e.g., log non-cancellation errors, or attempt a hard navigation fallback for non-cancel errors), this makes sense as a structural placeholder. But as-is, it could be simplified to a single routerEvents.emit("routeChangeError", err, fullAppUrl, { shallow: false }) without the if.

Same applies to the 4 identical try/catch blocks in push/replace.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is now moot — the instanceof check lives in runNavigateClient() once, not 4 times. The three-way return type ("completed" | "cancelled" | "failed") makes the branching explicit.

routerEvents.emit("routeChangeError", err, fullAppUrl, { shallow: false });
// For genuine errors (not cancellations), fall back to a hard navigation
// so the browser recovers to a consistent URL/content state.
if (!(err instanceof NavigationCancelledError)) {
window.location.href = browserUrl;
}
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Regression: The popstate rejection handler emits routeChangeError but doesn't fall back to hard navigation for non-cancellation errors.

The old code had window.location.href = url inside navigateClient()'s catch block, which recovered from unexpected failures (JSON parse errors, import failures, etc.) by doing a full page load. With this PR, those errors propagate out as throws. The 4 push/replace call sites catch them and return false, but the popstate handler only emits the event and stops — leaving the browser showing the popstate target URL with the old rendered content.

Consider adding a hard-navigation fallback for non-cancel errors here:

(err: unknown) => {
  routerEvents.emit("routeChangeError", err, fullAppUrl, { shallow: false });
  if (!(err instanceof NavigationCancelledError)) {
    window.location.href = browserUrl;
  }
},

The same gap exists in the 4 push/replace catch blocks — they return false but don't trigger a hard navigation, so the user is stuck on the old content with a new URL. In the old code, navigateClient handled this internally. Might be worth restoring that fallback inside navigateClient's own error path (for non-cancel errors) rather than relying on every caller to do it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. The popstate rejection handler now does window.location.href = browserUrl for non-cancel errors. Same fallback is in the new runNavigateClient() helper used by push/replace.

);
});
}

Expand Down Expand Up @@ -802,7 +924,9 @@ const Router = {
window.history.pushState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
const result = await runNavigateClient(full, resolved);
if (result === "cancelled") return true;
if (result === "failed") return false;
}
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });

Expand Down Expand Up @@ -852,7 +976,9 @@ const Router = {
window.history.replaceState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
const result = await runNavigateClient(full, resolved);
if (result === "cancelled") return true;
if (result === "failed") return false;
}
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });

Expand Down
Loading
Loading