Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 141 additions & 24 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,25 +386,57 @@ 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.
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]);
Expand All @@ -393,28 +456,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 +499,7 @@ async function navigateClient(url: string): Promise<void> {
}
}
}
assertStillCurrent();

let element;
if (AppComponent) {
Expand All @@ -448,12 +515,11 @@ async function navigateClient(url: string): Promise<void> {
element = wrapWithRouterContext(element);

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 433 to 529
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.

}

Expand Down Expand Up @@ -564,7 +630,18 @@ export function useRouter(): NextRouter {
window.history.pushState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
try {
await navigateClient(full);
} catch (err: unknown) {
const routeProps = { shallow: false };
routerEvents.emit("routeChangeError", err, resolved, routeProps);
// Cancelled means a newer navigation superseded this one — not a failure
// from the caller's perspective, so return true (matches Next.js behaviour).
if (err instanceof NavigationCancelledError) {
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.

When NavigationCancelledError is caught, the method returns true (success). This means calling code like const ok = await router.push('/page-a') gets true even though the navigation was cancelled and the content never loaded for /page-a.

Next.js's router.push also returns true for cancelled navigations (because the intent was replaced by a newer navigation, not because it failed), so this matches. But it might be worth a comment here explaining the rationale, since return true for a cancelled navigation is counterintuitive.

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.

The semantics are now documented in runNavigateClient()'s return type and JSDoc — "cancelled" is explicitly distinguished from "completed" and "failed", making the caller's intent clearer than a boolean.

return 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.

This identical 12-line try/catch block is duplicated 4 times: useRouter().push (here), useRouter().replace (line 701), Router.push (line 900), and Router.replace (line 961).

Consider extracting a helper:

async function runNavigateClient(
  full: string,
  resolved: string,
): Promise<{ ok: true } | { ok: false; cancelled: boolean }> {
  try {
    await navigateClient(full);
    return { ok: true };
  } catch (err: unknown) {
    routerEvents.emit("routeChangeError", err, resolved, { shallow: false });
    if (!(err instanceof NavigationCancelledError)) {
      window.location.href = full; // hard-navigation fallback
    }
    return { ok: false, cancelled: err instanceof NavigationCancelledError };
  }
}

This also gives a natural place to add the hard-navigation fallback for non-cancel errors (addressing point 2 above).

Not blocking, but strongly recommended before this file grows further.

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.

Done. Extracted runNavigateClient() returning "completed" | "cancelled" | "failed" so callers can distinguish all three outcomes. The 4 push/replace sites are now 3 lines each instead of 12.

}
return false;
}
}
setState(getPathnameAndQuery());
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });
Expand Down Expand Up @@ -621,7 +698,18 @@ export function useRouter(): NextRouter {
window.history.replaceState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
try {
await navigateClient(full);
} catch (err: unknown) {
const routeProps = { shallow: false };
routerEvents.emit("routeChangeError", err, resolved, routeProps);
// Cancelled means a newer navigation superseded this one — not a failure
// from the caller's perspective, so return true (matches Next.js behaviour).
if (err instanceof NavigationCancelledError) {
return true;
}
return false;
}
}
setState(getPathnameAndQuery());
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });
Expand Down Expand Up @@ -730,11 +818,18 @@ 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.

// Both real errors and cancellations emit routeChangeError. The popstate
// handler has no return value so there is no true/false distinction here.
routerEvents.emit("routeChangeError", err, fullAppUrl, { shallow: false });
},
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 +897,18 @@ const Router = {
window.history.pushState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
try {
await navigateClient(full);
} catch (err: unknown) {
const routeProps = { shallow: false };
routerEvents.emit("routeChangeError", err, resolved, routeProps);
// Cancelled means a newer navigation superseded this one — not a failure
// from the caller's perspective, so return true (matches Next.js behaviour).
if (err instanceof NavigationCancelledError) {
return true;
}
return false;
}
}
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });

Expand Down Expand Up @@ -852,7 +958,18 @@ const Router = {
window.history.replaceState({}, "", full);
_lastPathnameAndSearch = window.location.pathname + window.location.search;
if (!options?.shallow) {
await navigateClient(full);
try {
await navigateClient(full);
} catch (err: unknown) {
const routeProps = { shallow: false };
routerEvents.emit("routeChangeError", err, resolved, routeProps);
// Cancelled means a newer navigation superseded this one — not a failure
// from the caller's perspective, so return true (matches Next.js behaviour).
if (err instanceof NavigationCancelledError) {
return true;
}
return false;
}
}
routerEvents.emit("routeChangeComplete", resolved, { shallow: options?.shallow ?? false });

Expand Down
Loading
Loading