-
Notifications
You must be signed in to change notification settings - Fork 294
fix: prevent URL/content mismatch on rapid Pages Router navigation #782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
dd8c624
f778364
832d6c7
8a07411
4aef8f2
1539d8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
| 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; | ||
|
|
||
|
|
@@ -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(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 One edge case to consider: if |
||
| const controller = new AbortController(); | ||
| _activeAbortController = controller; | ||
|
|
||
| const navId = ++_navigationId; | ||
|
|
||
| /** Check if this navigation is still the active one. If not, throw. */ | ||
| function assertStillCurrent(): void { | ||
| 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good comment explaining the Worth adding a note like:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a note: |
||
| return; | ||
| const err = new Error(`Navigation failed: ${res.status} ${res.statusText}`); | ||
| throw err; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ordering concern: In the old code, these were early |
||
|
|
||
| const html = await res.text(); | ||
| assertStillCurrent(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug (low-severity):
Between steps 2 and 4, Fix: Defer
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Deferred the |
||
|
|
||
| // 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 | ||
|
|
@@ -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__; | ||
|
|
@@ -433,6 +502,7 @@ async function navigateClient(url: string): Promise<void> { | |
| } | ||
| } | ||
| } | ||
| assertStillCurrent(); | ||
|
|
||
| let element; | ||
| if (AppComponent) { | ||
|
|
@@ -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 { | ||
| _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
|
||
| } | ||
|
|
||
| /** | ||
| * 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; | ||
| } | ||
| return "failed"; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -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 }); | ||
|
|
@@ -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 }); | ||
|
|
@@ -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) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the Same applies to the 4 identical try/catch blocks in push/replace.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now moot — the |
||
| 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; | ||
| } | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regression: The popstate rejection handler emits The old code had 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. The popstate rejection handler now does |
||
| ); | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -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 }); | ||
|
|
||
|
|
@@ -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 }); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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 inrouteChangeErrorlisteners 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.
There was a problem hiding this comment.
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.