From 19ca284046b56e91d60cb5037905c0fc41eed8c4 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 19 Nov 2025 12:25:48 -0500 Subject: [PATCH 1/3] Refactor how router errors are reported to avoid strict mode issues --- .changeset/long-brooms-shake.md | 5 +++ packages/react-router/lib/components.tsx | 47 +++++++++------------- packages/react-router/lib/router/router.ts | 2 + 3 files changed, 26 insertions(+), 28 deletions(-) create mode 100644 .changeset/long-brooms-shake.md diff --git a/.changeset/long-brooms-shake.md b/.changeset/long-brooms-shake.md new file mode 100644 index 0000000000..9d272f25b1 --- /dev/null +++ b/.changeset/long-brooms-shake.md @@ -0,0 +1,5 @@ +--- +"react-router": patch +--- + +[UNSTABLE] Internal refactor to how `unstable_onError` is called by `RouterProvider` diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx index 4be51e5cc5..93730d7711 100644 --- a/packages/react-router/lib/components.tsx +++ b/packages/react-router/lib/components.tsx @@ -542,31 +542,22 @@ export function RouterProvider({ nextLocation: Location; }>(); let fetcherData = React.useRef>(new Map()); - let logErrorsAndSetState = React.useCallback( - (newState: RouterState) => { - setStateImpl((prevState) => { - // Send loader/action errors through handleError - if (newState.errors && unstable_onError) { - Object.entries(newState.errors).forEach(([routeId, error]) => { - if (prevState.errors?.[routeId] !== error) { - unstable_onError(error, { - location: newState.location, - params: newState.matches[0]?.params ?? {}, - }); - } - }); - } - return newState; - }); - }, - [unstable_onError], - ); let setState = React.useCallback( ( newState: RouterState, - { deletedFetchers, flushSync, viewTransitionOpts }, + { deletedFetchers, newErrors, flushSync, viewTransitionOpts }, ) => { + // Send router errors through onError + if (newErrors && unstable_onError) { + Object.values(newErrors).forEach((error) => + unstable_onError(error, { + location: newState.location, + params: newState.matches[0]?.params ?? {}, + }), + ); + } + newState.fetchers.forEach((fetcher, key) => { if (fetcher.data !== undefined) { fetcherData.current.set(key, fetcher.data); @@ -600,9 +591,9 @@ export function RouterProvider({ // just update and be done with it if (!viewTransitionOpts || !isViewTransitionAvailable) { if (reactDomFlushSyncImpl && flushSync) { - reactDomFlushSyncImpl(() => logErrorsAndSetState(newState)); + reactDomFlushSyncImpl(() => setStateImpl(newState)); } else { - React.startTransition(() => logErrorsAndSetState(newState)); + React.startTransition(() => setStateImpl(newState)); } return; } @@ -613,7 +604,7 @@ export function RouterProvider({ reactDomFlushSyncImpl(() => { // Cancel any pending transitions if (transition) { - renderDfd && renderDfd.resolve(); + renderDfd?.resolve(); transition.skipTransition(); } setVtContext({ @@ -626,7 +617,7 @@ export function RouterProvider({ // Update the DOM let t = router.window!.document.startViewTransition(() => { - reactDomFlushSyncImpl(() => logErrorsAndSetState(newState)); + reactDomFlushSyncImpl(() => setStateImpl(newState)); }); // Clean up after the animation completes @@ -647,7 +638,7 @@ export function RouterProvider({ if (transition) { // Interrupting an in-progress transition, cancel and let everything flush // out, and then kick off a new transition from the interruption state - renderDfd && renderDfd.resolve(); + renderDfd?.resolve(); transition.skipTransition(); setInterruption({ state: newState, @@ -670,7 +661,7 @@ export function RouterProvider({ reactDomFlushSyncImpl, transition, renderDfd, - logErrorsAndSetState, + unstable_onError, ], ); @@ -694,7 +685,7 @@ export function RouterProvider({ let newState = pendingState; let renderPromise = renderDfd.promise; let transition = router.window.document.startViewTransition(async () => { - React.startTransition(() => logErrorsAndSetState(newState)); + React.startTransition(() => setStateImpl(newState)); await renderPromise; }); transition.finished.finally(() => { @@ -705,7 +696,7 @@ export function RouterProvider({ }); setTransition(transition); } - }, [pendingState, renderDfd, router.window, logErrorsAndSetState]); + }, [pendingState, renderDfd, router.window]); // When the new location finally renders and is committed to the DOM, this // effect will run to resolve the transition diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index 0cee95df0a..d26368f286 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -489,6 +489,7 @@ export interface RouterSubscriber { state: RouterState, opts: { deletedFetchers: string[]; + newErrors: RouteData | null; viewTransitionOpts?: ViewTransitionOpts; flushSync: boolean; }, @@ -1292,6 +1293,7 @@ export function createRouter(init: RouterInit): Router { [...subscribers].forEach((subscriber) => subscriber(state, { deletedFetchers: unmountedFetchers, + newErrors: newState.errors ?? null, viewTransitionOpts: opts.viewTransitionOpts, flushSync: opts.flushSync === true, }), From 115f087fed19059e9209f4480f4e5ff5225b0ba5 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 20 Nov 2025 14:54:20 -0500 Subject: [PATCH 2/3] Add unstable_pattern to client onError --- .../__tests__/dom/client-on-error-test.tsx | 16 ++++++++++++++++ packages/react-router/lib/components.tsx | 12 ++++++++++-- packages/react-router/lib/hooks.tsx | 2 ++ packages/react-router/lib/router/router.ts | 12 ++++++------ packages/react-router/lib/router/utils.ts | 10 ++++++++-- 5 files changed, 42 insertions(+), 10 deletions(-) diff --git a/packages/react-router/__tests__/dom/client-on-error-test.tsx b/packages/react-router/__tests__/dom/client-on-error-test.tsx index 9a26bded73..09bdd48d31 100644 --- a/packages/react-router/__tests__/dom/client-on-error-test.tsx +++ b/packages/react-router/__tests__/dom/client-on-error-test.tsx @@ -51,6 +51,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("lazy error!"), { location: expect.objectContaining({ pathname: "/" }), params: {}, + unstable_pattern: "/", }); expect(spy).toHaveBeenCalledTimes(1); expect(getHtml(container)).toContain("Unexpected Application Error!"); @@ -81,6 +82,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("middleware error!"), { location: expect.objectContaining({ pathname: "/" }), params: {}, + unstable_pattern: "/", }); expect(spy).toHaveBeenCalledTimes(1); }); @@ -109,6 +111,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("loader error!"), { location: expect.objectContaining({ pathname: "/" }), params: {}, + unstable_pattern: "/", }); expect(spy).toHaveBeenCalledTimes(1); }); @@ -139,6 +142,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("lazy error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", }); expect(spy).toHaveBeenCalledTimes(1); let html = getHtml(container); @@ -174,6 +178,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("middleware error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", }); expect(spy).toHaveBeenCalledTimes(1); expect(getHtml(container)).toContain("Error"); @@ -205,6 +210,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("loader error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", }); expect(spy).toHaveBeenCalledTimes(1); expect(getHtml(container)).toContain("Error"); @@ -241,6 +247,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("action error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", }); expect(spy).toHaveBeenCalledTimes(1); expect(getHtml(container)).toContain("Error"); @@ -270,6 +277,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("loader error!"), { location: expect.objectContaining({ pathname: "/" }), params: {}, + unstable_pattern: "/", }); expect(spy).toHaveBeenCalledTimes(1); expect(getHtml(container)).toContain("Error"); @@ -304,6 +312,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("action error!"), { location: expect.objectContaining({ pathname: "/" }), params: {}, + unstable_pattern: "/", }); expect(spy).toHaveBeenCalledTimes(1); expect(getHtml(container)).toContain("Error"); @@ -334,6 +343,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("render error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", errorInfo: expect.objectContaining({ componentStack: expect.any(String), }), @@ -379,6 +389,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("await error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", }); expect(spy).toHaveBeenCalledTimes(1); expect(getHtml(container)).toContain("Await Error"); @@ -427,6 +438,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("await error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", errorInfo: expect.objectContaining({ componentStack: expect.any(String), }), @@ -481,10 +493,12 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("await error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", }); expect(spy).toHaveBeenCalledWith(new Error("errorElement error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", errorInfo: expect.objectContaining({ componentStack: expect.any(String), }), @@ -534,6 +548,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("loader error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", }); expect(spy).toHaveBeenCalledTimes(1); expect(getHtml(container)).toContain("Error"); @@ -584,6 +599,7 @@ describe(`handleError`, () => { expect(spy).toHaveBeenCalledWith(new Error("render error!"), { location: expect.objectContaining({ pathname: "/page" }), params: {}, + unstable_pattern: "/page", errorInfo: expect.objectContaining({ componentStack: expect.any(String), }), diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx index 93730d7711..e4ec6e080e 100644 --- a/packages/react-router/lib/components.tsx +++ b/packages/react-router/lib/components.tsx @@ -29,7 +29,12 @@ import type { Params, TrackedPromise, } from "./router/utils"; -import { getResolveToMatches, resolveTo, stripBasename } from "./router/utils"; +import { + getResolveToMatches, + getRoutePattern, + resolveTo, + stripBasename, +} from "./router/utils"; import type { DataRouteObject, @@ -313,6 +318,7 @@ export interface unstable_ClientOnErrorFunction { info: { location: Location; params: Params; + unstable_pattern: string; errorInfo?: React.ErrorInfo; }, ): void; @@ -554,6 +560,7 @@ export function RouterProvider({ unstable_onError(error, { location: newState.location, params: newState.matches[0]?.params ?? {}, + unstable_pattern: getRoutePattern(newState.matches), }), ); } @@ -1610,7 +1617,8 @@ export function Await({ ) { dataRouterContext.unstable_onError(error, { location: dataRouterStateContext.location, - params: dataRouterStateContext.matches?.[0]?.params || {}, + params: dataRouterStateContext.matches[0]?.params || {}, + unstable_pattern: getRoutePattern(dataRouterStateContext.matches), errorInfo, }); } diff --git a/packages/react-router/lib/hooks.tsx b/packages/react-router/lib/hooks.tsx index 2f90ca381f..83bd86f734 100644 --- a/packages/react-router/lib/hooks.tsx +++ b/packages/react-router/lib/hooks.tsx @@ -43,6 +43,7 @@ import { convertRouteMatchToUiMatch, decodePath, getResolveToMatches, + getRoutePattern, isRouteErrorResponse, joinPaths, matchPath, @@ -1194,6 +1195,7 @@ export function _renderMatches( unstable_onError(error, { location: dataRouterState.location, params: dataRouterState.matches?.[0]?.params ?? {}, + unstable_pattern: getRoutePattern(dataRouterState.matches), errorInfo, }); } diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index d26368f286..6e900b7c5c 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -3768,7 +3768,7 @@ export function createStaticHandler( let response = await runServerMiddlewarePipeline( { request, - unstable_pattern: getRoutePattern(matches.map((m) => m.route.path)), + unstable_pattern: getRoutePattern(matches), matches, params: matches[0].params, // If we're calling middleware then it must be enabled so we can cast @@ -4000,7 +4000,7 @@ export function createStaticHandler( let response = await runServerMiddlewarePipeline( { request, - unstable_pattern: getRoutePattern(matches.map((m) => m.route.path)), + unstable_pattern: getRoutePattern(matches), matches, params: matches[0].params, // If we're calling middleware then it must be enabled so we can cast @@ -4393,7 +4393,7 @@ export function createStaticHandler( matches.findIndex((m) => m.route.id === pendingActionResult[0]) - 1 : undefined; - let pattern = getRoutePattern(matches.map((m) => m.route.path)); + let pattern = getRoutePattern(matches); dsMatches = matches.map((match, index) => { if (maxIdx != null && index > maxIdx) { return getDataStrategyMatch( @@ -4867,7 +4867,7 @@ function getMatchesToLoad( actionStatus, }; - let pattern = getRoutePattern(matches.map((m) => m.route.path)); + let pattern = getRoutePattern(matches); let dsMatches: DataStrategyMatch[] = matches.map((match, index) => { let { route } = match; @@ -5934,7 +5934,7 @@ function getTargetedDataStrategyMatches( mapRouteProperties, manifest, request, - getRoutePattern(matches.map((m) => m.route.path)), + getRoutePattern(matches), match, lazyRoutePropertiesToSkip, scopedContext, @@ -5962,7 +5962,7 @@ async function callDataStrategyImpl( // back out below. let dataStrategyArgs = { request, - unstable_pattern: getRoutePattern(matches.map((m) => m.route.path)), + unstable_pattern: getRoutePattern(matches), params: matches[0].params, context: scopedContext, matches, diff --git a/packages/react-router/lib/router/utils.ts b/packages/react-router/lib/router/utils.ts index f90aba951f..b670d47781 100644 --- a/packages/react-router/lib/router/utils.ts +++ b/packages/react-router/lib/router/utils.ts @@ -2049,6 +2049,12 @@ by the star-slash in the `getRoutePattern` regex and messes up the parsed commen for `isRouteErrorResponse` above. This comment seems to reset the parser. */ -export function getRoutePattern(paths: (string | undefined)[]) { - return paths.filter(Boolean).join("/").replace(/\/\/*/g, "/") || "/"; +export function getRoutePattern(matches: AgnosticRouteMatch[]) { + return ( + matches + .map((m) => m.route.path) + .filter(Boolean) + .join("/") + .replace(/\/\/*/g, "/") || "/" + ); } From 82a7c7dfff9e861fd78fc03610be6aa2c338a395 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 20 Nov 2025 14:55:53 -0500 Subject: [PATCH 3/3] Apply suggestion from @brophdawg11 --- .changeset/long-brooms-shake.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/long-brooms-shake.md b/.changeset/long-brooms-shake.md index 9d272f25b1..6d78c7abae 100644 --- a/.changeset/long-brooms-shake.md +++ b/.changeset/long-brooms-shake.md @@ -2,4 +2,4 @@ "react-router": patch --- -[UNSTABLE] Internal refactor to how `unstable_onError` is called by `RouterProvider` +[UNSTABLE] Add `unstable_pattern` to the parameters for client side `unstable_onError`, refactor how it's called by `RouterProvider` to avoid potential strict mode issues