Minor refactor and add unstable_pattern to client onError

.changeset/long-brooms-shake.md

@@ -0,0 +1,5 @@
+---
+"react-router": patch
+---
+
+[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

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),
       }),

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;
@@ -542,31 +548,23 @@ export function RouterProvider({
     nextLocation: Location;
   }>();
   let fetcherData = React.useRef<Map<string, any>>(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<RouterSubscriber>(
     (
       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 ?? {},
+            unstable_pattern: getRoutePattern(newState.matches),
+          }),
+        );
+      }
+
       newState.fetchers.forEach((fetcher, key) => {
         if (fetcher.data !== undefined) {
           fetcherData.current.set(key, fetcher.data);
@@ -600,9 +598,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 +611,7 @@ export function RouterProvider({
         reactDomFlushSyncImpl(() => {
           // Cancel any pending transitions
           if (transition) {
-            renderDfd && renderDfd.resolve();
+            renderDfd?.resolve();
             transition.skipTransition();
           }
           setVtContext({
@@ -626,7 +624,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 +645,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 +668,7 @@ export function RouterProvider({
       reactDomFlushSyncImpl,
       transition,
       renderDfd,
-      logErrorsAndSetState,
+      unstable_onError,
     ],
   );
 
@@ -694,7 +692,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 +703,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
@@ -1619,7 +1617,8 @@ export function Await<Resolve>({
       ) {
         dataRouterContext.unstable_onError(error, {
           location: dataRouterStateContext.location,
-          params: dataRouterStateContext.matches?.[0]?.params || {},
+          params: dataRouterStateContext.matches[0]?.params || {},
+          unstable_pattern: getRoutePattern(dataRouterStateContext.matches),
           errorInfo,
         });
       }

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,
           });
         }

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,
       }),
@@ -3766,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
@@ -3998,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
@@ -4391,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(
@@ -4865,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;
 
@@ -5932,7 +5934,7 @@ function getTargetedDataStrategyMatches(
       mapRouteProperties,
       manifest,
       request,
-      getRoutePattern(matches.map((m) => m.route.path)),
+      getRoutePattern(matches),
       match,
       lazyRoutePropertiesToSkip,
       scopedContext,
@@ -5960,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,

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, "/") || "/"
+  );
 }