diff --git a/.changeset/fetcher-abort-fix.md b/.changeset/fetcher-abort-fix.md new file mode 100644 index 0000000000..f1a7241774 --- /dev/null +++ b/.changeset/fetcher-abort-fix.md @@ -0,0 +1,5 @@ +--- +"react-router": patch +--- + +Prevent abort-related fetcher errors from surfacing as navigation failures. diff --git a/contributors.yml b/contributors.yml index 58131406b9..986689a78a 100644 --- a/contributors.yml +++ b/contributors.yml @@ -464,6 +464,8 @@ - xcsnowcity - xdaxer - yionr +- Yonatani +- yoni-noma - yracnet - ytori - yuhwan-park diff --git a/integration/fetcher-test.ts b/integration/fetcher-test.ts index 3a04da1ff8..35dc54430f 100644 --- a/integration/fetcher-test.ts +++ b/integration/fetcher-test.ts @@ -88,6 +88,43 @@ test.describe("useFetcher", () => { } `, + "app/routes/fetcher-abort.tsx": js` + import { Link, useFetcher } from "react-router"; + import { useEffect, useState } from "react"; + + export async function loader() { + await new Promise(resolve => setTimeout(resolve, 150)); + return { ok: true }; + } + + export default function FetcherAbort() { + let fetcher = useFetcher(); + let [polling, setPolling] = useState(false); + + useEffect(() => { + if (!polling) return; + let intervalId = setInterval(() => { + fetcher.load("/fetcher-abort"); + }, 50); + return () => clearInterval(intervalId); + }, [polling, fetcher]); + + return ( +
+ + Go to Target +

{fetcher.state}

+
+ ); + } + `, + + "app/routes/target.tsx": js` + export default function Target() { + return

Target

; + } + `, + "app/routes/parent.tsx": js` import { Outlet } from "react-router"; @@ -415,6 +452,18 @@ test.describe("useFetcher", () => { ]), ); }); + + test("navigation during fetcher polling does not error", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + + await app.goto("/fetcher-abort"); + await page.getByRole("button", { name: "Start Polling" }).click(); + await page.waitForTimeout(200); + + await page.getByRole("link", { name: "Go to Target" }).click(); + await expect(page).toHaveURL(/\/target/); + await expect(page.getByRole("heading", { name: "Target" })).toBeVisible(); + }); }); test.describe("fetcher aborts and adjacent forms", () => { diff --git a/packages/react-router/__tests__/router/data-strategy-test.ts b/packages/react-router/__tests__/router/data-strategy-test.ts index 033d66b4c0..32b493461e 100644 --- a/packages/react-router/__tests__/router/data-strategy-test.ts +++ b/packages/react-router/__tests__/router/data-strategy-test.ts @@ -792,6 +792,63 @@ describe("router dataStrategy", () => { }); }); + describe("aborts", () => { + it("returns AbortError results when a navigation is interrupted", async () => { + let resultsByPathname = new Map< + string, + Record + >(); + let dataStrategy = mockDataStrategy(({ matches, request }) => { + return Promise.all(matches.map((m) => m.resolve())).then((results) => { + let keyed = keyedResults(matches, results); + resultsByPathname.set(new URL(request.url).pathname, keyed); + return keyed; + }); + }); + let t = setup({ + routes: [ + { + path: "/", + id: "root", + loader: true, + children: [ + { + id: "foo", + path: "foo", + loader: true, + }, + { + id: "bar", + path: "bar", + loader: true, + }, + ], + }, + ], + dataStrategy, + hydrationData: { + loaderData: { + root: "ROOT", + }, + }, + }); + + let A = await t.navigate("/foo"); + let B = await t.navigate("/bar"); + await B.loaders.root.resolve("ROOT*"); + await B.loaders.bar.resolve("BAR"); + await tick(); + + expect(A.loaders.foo.signal.aborted).toBe(true); + let abortedResults = resultsByPathname.get("/foo"); + expect(abortedResults).toBeDefined(); + expect(abortedResults?.foo?.type).toBe("error"); + expect(abortedResults?.foo?.result).toEqual( + expect.objectContaining({ name: "AbortError" }), + ); + }); + }); + describe("actions", () => { it("should allow a custom implementation to passthrough to default behavior", async () => { let dataStrategy = mockDataStrategy(({ matches }) => diff --git a/packages/react-router/lib/dom/ssr/fog-of-war.ts b/packages/react-router/lib/dom/ssr/fog-of-war.ts index 7541bd5383..9567572e34 100644 --- a/packages/react-router/lib/dom/ssr/fog-of-war.ts +++ b/packages/react-router/lib/dom/ssr/fog-of-war.ts @@ -303,7 +303,9 @@ export async function fetchAndApplyManifestPatches( } serverPatches = (await res.json()) as AssetsManifest["routes"]; } catch (e) { - if (signal?.aborted) return; + if (signal?.aborted) { + return; + } throw e; } diff --git a/packages/react-router/lib/dom/ssr/single-fetch.tsx b/packages/react-router/lib/dom/ssr/single-fetch.tsx index fdf78ff273..3b51f7c24b 100644 --- a/packages/react-router/lib/dom/ssr/single-fetch.tsx +++ b/packages/react-router/lib/dom/ssr/single-fetch.tsx @@ -15,6 +15,7 @@ import { data, stripBasename, } from "../../router/utils"; +import { isAbortError } from "../../router/abort"; import { createRequestInit } from "./data"; import type { AssetsManifest, EntryContext } from "./entry"; import { escapeHtml } from "./markup"; @@ -663,7 +664,15 @@ async function fetchAndDecodeViaTurboStream( } } - let res = await fetch(url, await createRequestInit(request)); + let res: Response; + try { + res = await fetch(url, await createRequestInit(request)); + } catch (e) { + if (isAbortError(e, request.signal, { allowTypeError: true })) { + throw new DOMException("Aborted", "AbortError"); + } + throw e; + } // If this error'd without hitting the running server, then bubble a normal // `ErrorResponse` and don't try to decode the body with `turbo-stream`. diff --git a/packages/react-router/lib/router/abort.ts b/packages/react-router/lib/router/abort.ts new file mode 100644 index 0000000000..11a4c0cc72 --- /dev/null +++ b/packages/react-router/lib/router/abort.ts @@ -0,0 +1,45 @@ +type AbortErrorOptions = { + allowTypeError?: boolean; +}; + +export function isAbortError( + error: unknown, + signal: AbortSignal, + options: AbortErrorOptions = {}, +) { + const { allowTypeError = true } = options; + if (signal.aborted) { + return true; + } + + const hasDomException = typeof DOMException !== "undefined"; + const signalReason = signal.reason; + + if ( + hasDomException && + signalReason instanceof DOMException && + signalReason.name === "AbortError" + ) { + return true; + } + if (signalReason instanceof Error && signalReason.name === "AbortError") { + return true; + } + + if (hasDomException && error instanceof DOMException) { + return error.name === "AbortError"; + } + if (error instanceof TypeError) { + if (!allowTypeError) { + return false; + } + // Fallback for browsers that surface aborted fetches as TypeError + return /failed to fetch|load failed|network request failed|the operation was aborted/i.test( + error.message, + ); + } + if (error instanceof Error) { + return error.name === "AbortError"; + } + return false; +} diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index 34c74cc28d..28ac2ede49 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -9,6 +9,7 @@ import { parsePath, warning, } from "./history"; +import { isAbortError } from "./abort"; import type { unstable_ClientInstrumentation, unstable_InstrumentRouteFunction, @@ -1227,7 +1228,9 @@ export function createRouter(init: RouterInit): Router { removePageHideEventListener(); } subscribers.clear(); - pendingNavigationController && pendingNavigationController.abort(); + if (pendingNavigationController) { + pendingNavigationController.abort(); + } state.fetchers.forEach((_, key) => deleteFetcher(key)); state.blockers.forEach((_, key) => deleteBlocker(key)); } @@ -1676,7 +1679,9 @@ export function createRouter(init: RouterInit): Router { // Abort any in-progress navigations and start a new one. Unset any ongoing // uninterrupted revalidations unless told otherwise, since we want this // new navigation to update history normally - pendingNavigationController && pendingNavigationController.abort(); + if (pendingNavigationController) { + pendingNavigationController.abort(); + } pendingNavigationController = null; pendingAction = historyAction; isUninterruptedRevalidation = @@ -2209,7 +2214,7 @@ export function createRouter(init: RouterInit): Router { } revalidatingFetchers.forEach((rf) => { - abortFetcher(rf.key); + abortFetcher(rf.key, "revalidation:reset"); if (rf.controller) { // Fetchers use an independent AbortController so that aborting a fetcher // (via deleteFetcher) does not abort the triggering navigation that @@ -2220,7 +2225,9 @@ export function createRouter(init: RouterInit): Router { // Proxy navigation abort through to revalidation fetchers let abortPendingFetchRevalidations = () => - revalidatingFetchers.forEach((f) => abortFetcher(f.key)); + revalidatingFetchers.forEach((f) => + abortFetcher(f.key, "revalidation:navigation-abort"), + ); if (pendingNavigationController) { pendingNavigationController.signal.addEventListener( "abort", @@ -2341,7 +2348,7 @@ export function createRouter(init: RouterInit): Router { href: string | null, opts?: RouterFetchOptions, ) { - abortFetcher(key); + abortFetcher(key, "fetcher:start"); let flushSync = (opts && opts.flushSync) === true; @@ -2462,6 +2469,13 @@ export function createRouter(init: RouterInit): Router { if (discoverResult.type === "aborted") { return; } else if (discoverResult.type === "error") { + if ( + isAbortError(discoverResult.error, fetchRequest.signal, { + allowTypeError: false, + }) + ) { + return; + } setFetcherError(key, routeId, discoverResult.error, { flushSync }); return; } else if (!discoverResult.matches) { @@ -2530,6 +2544,14 @@ export function createRouter(init: RouterInit): Router { return; } + // If the result was skipped by abort detection in callDataStrategy, + // gracefully settle the fetcher with its previous data instead of crashing + if (!actionResult) { + let currentFetcher = state.fetchers.get(key); + updateFetcherState(key, getDoneFetcher(currentFetcher?.data)); + return; + } + // We don't want errors bubbling up to the UI or redirects processed for // unmounted fetchers so we just revert them to idle if (fetchersQueuedForDeletion.has(key)) { @@ -2624,7 +2646,7 @@ export function createRouter(init: RouterInit): Router { existingFetcher ? existingFetcher.data : undefined, ); state.fetchers.set(staleKey, revalidatingFetcher); - abortFetcher(staleKey); + abortFetcher(staleKey, "revalidation:stale-fetcher"); if (rf.controller) { fetchControllers.set(staleKey, rf.controller); } @@ -2633,7 +2655,9 @@ export function createRouter(init: RouterInit): Router { updateState({ fetchers: new Map(state.fetchers) }); let abortPendingFetchRevalidations = () => - revalidatingFetchers.forEach((rf) => abortFetcher(rf.key)); + revalidatingFetchers.forEach((rf) => + abortFetcher(rf.key, "revalidation:action-abort"), + ); abortController.signal.addEventListener( "abort", @@ -2712,7 +2736,9 @@ export function createRouter(init: RouterInit): Router { loadId > pendingNavigationLoadId ) { invariant(pendingAction, "Expected pending action"); - pendingNavigationController && pendingNavigationController.abort(); + if (pendingNavigationController) { + pendingNavigationController.abort(); + } completeNavigation(state.navigation.location, { matches, @@ -2778,6 +2804,13 @@ export function createRouter(init: RouterInit): Router { if (discoverResult.type === "aborted") { return; } else if (discoverResult.type === "error") { + if ( + isAbortError(discoverResult.error, fetchRequest.signal, { + allowTypeError: false, + }) + ) { + return; + } setFetcherError(key, routeId, discoverResult.error, { flushSync }); return; } else if (!discoverResult.matches) { @@ -2833,6 +2866,14 @@ export function createRouter(init: RouterInit): Router { return; } + // If the result was skipped by abort detection in callDataStrategy, + // gracefully settle the fetcher with its previous data instead of crashing + if (result == null) { + let currentFetcher = state.fetchers.get(key); + updateFetcherState(key, getDoneFetcher(currentFetcher?.data)); + return; + } + // If the loader threw a redirect Response, start a new REPLACE navigation if (isRedirectResult(result)) { if (pendingNavigationLoadId > originatingLoadId) { @@ -3027,6 +3068,12 @@ export function createRouter(init: RouterInit): Router { false, ); } catch (e) { + // If the request was aborted, don't treat it as an error - just return + // empty results. This prevents abort errors from bubbling to error boundary. + // See: https://github.com/remix-run/react-router/issues/14203 + if (isAbortError(e, request.signal, { allowTypeError: fetcherKey != null })) { + return dataResults; + } // If the outer dataStrategy method throws, just return the error for all // matches - and it'll naturally bubble to the root matches @@ -3071,6 +3118,19 @@ export function createRouter(init: RouterInit): Router { } for (let [routeId, result] of Object.entries(results)) { + // If this is an abort-related error result, skip it entirely so + // mergeLoaderData preserves the existing valid data for this route. + // Writing { data: undefined } would wipe loaderData and crash hooks + // like useRouteLoaderData('root') that expect data to be present. + // See: https://github.com/remix-run/react-router/issues/14203 + if ( + result.type === ResultType.error && + isAbortError(result.result, request.signal, { + allowTypeError: fetcherKey != null, + }) + ) { + continue; + } if (isRedirectDataStrategyResult(result)) { let response = result.result as Response; dataResults[routeId] = { @@ -3116,6 +3176,18 @@ export function createRouter(init: RouterInit): Router { f.key, ); let result = results[f.match.route.id]; + // If the result was skipped by abort detection in callDataStrategy, + // synthesize a success result with previous data so downstream + // invariant checks don't crash + if (result == null) { + let existingFetcher = state.fetchers.get(f.key); + return { + [f.key]: { + type: ResultType.data, + data: existingFetcher?.data, + } as SuccessResult, + }; + } // Fetcher results are keyed by fetcher key from here on out, not routeId return { [f.key]: result }; } else { @@ -3152,7 +3224,7 @@ export function createRouter(init: RouterInit): Router { if (fetchControllers.has(key)) { cancelledFetcherLoads.add(key); } - abortFetcher(key); + abortFetcher(key, "interruptActiveLoads"); }); } @@ -3198,7 +3270,7 @@ export function createRouter(init: RouterInit): Router { } function resetFetcher(key: string, opts?: { reason?: unknown }) { - abortFetcher(key, opts?.reason); + abortFetcher(key, opts?.reason ?? "fetcher:reset"); updateFetcherState(key, getDoneFetcher(null)); } @@ -3211,7 +3283,7 @@ export function createRouter(init: RouterInit): Router { fetchControllers.has(key) && !(fetcher && fetcher.state === "loading" && fetchReloadIds.has(key)) ) { - abortFetcher(key); + abortFetcher(key, "fetcher:delete"); } fetchLoadMatches.delete(key); fetchReloadIds.delete(key); @@ -3271,7 +3343,7 @@ export function createRouter(init: RouterInit): Router { let fetcher = state.fetchers.get(key); invariant(fetcher, `Expected fetcher: ${key}`); if (fetcher.state === "loading") { - abortFetcher(key); + abortFetcher(key, "stale-fetch-load"); fetchReloadIds.delete(key); yeetedKeys.push(key); } @@ -6163,11 +6235,28 @@ async function callLoaderOrAction({ handler: boolean | LoaderFunction | ActionFunction, ): Promise => { // Setup a promise we can race against so that abort signals short circuit - let reject: () => void; + let reject: (reason?: unknown) => void; // This will never resolve so safe to type it as Promise to // satisfy the function return value let abortPromise = new Promise((_, r) => (reject = r)); - onReject = () => reject(); + onReject = () => { + const reason = request.signal.reason; + if ( + reason instanceof Error || + (typeof DOMException !== "undefined" && reason instanceof DOMException) + ) { + reject(reason); + return; + } + const abortMessage = typeof reason === "string" ? reason : "Aborted"; + if (typeof DOMException !== "undefined") { + reject(new DOMException(abortMessage, "AbortError")); + } else { + const abortError = new Error(abortMessage); + abortError.name = "AbortError"; + reject(abortError); + } + }; request.signal.addEventListener("abort", onReject); let actualHandler = (ctx?: unknown) => { diff --git a/packages/react-router/package.json b/packages/react-router/package.json index edfe9a3599..b82ce73ec9 100644 --- a/packages/react-router/package.json +++ b/packages/react-router/package.json @@ -1,6 +1,6 @@ { "name": "react-router", - "version": "7.13.0", + "version": "7.13.1", "description": "Declarative routing for React", "keywords": [ "react",