Skip to content

Commit b882394

Browse files
brophdawg11brophdawg11
authored
Minor refactor and add unstable_pattern to client onError (#14573)
1 parent 0e27357 commit b882394

File tree

6 files changed

+68
-38
lines changed

6 files changed

+68
-38
lines changed

‎.changeset/long-brooms-shake.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"react-router": patch
3+
---
4+
5+
[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‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ describe(`handleError`, () => {
5151
expect(spy).toHaveBeenCalledWith(new Error("lazy error!"), {
5252
location: expect.objectContaining({ pathname: "/" }),
5353
params: {},
54+
unstable_pattern: "/",
5455
});
5556
expect(spy).toHaveBeenCalledTimes(1);
5657
expect(getHtml(container)).toContain("Unexpected Application Error!");
@@ -81,6 +82,7 @@ describe(`handleError`, () => {
8182
expect(spy).toHaveBeenCalledWith(new Error("middleware error!"), {
8283
location: expect.objectContaining({ pathname: "/" }),
8384
params: {},
85+
unstable_pattern: "/",
8486
});
8587
expect(spy).toHaveBeenCalledTimes(1);
8688
});
@@ -109,6 +111,7 @@ describe(`handleError`, () => {
109111
expect(spy).toHaveBeenCalledWith(new Error("loader error!"), {
110112
location: expect.objectContaining({ pathname: "/" }),
111113
params: {},
114+
unstable_pattern: "/",
112115
});
113116
expect(spy).toHaveBeenCalledTimes(1);
114117
});
@@ -139,6 +142,7 @@ describe(`handleError`, () => {
139142
expect(spy).toHaveBeenCalledWith(new Error("lazy error!"), {
140143
location: expect.objectContaining({ pathname: "/page" }),
141144
params: {},
145+
unstable_pattern: "/page",
142146
});
143147
expect(spy).toHaveBeenCalledTimes(1);
144148
let html = getHtml(container);
@@ -174,6 +178,7 @@ describe(`handleError`, () => {
174178
expect(spy).toHaveBeenCalledWith(new Error("middleware error!"), {
175179
location: expect.objectContaining({ pathname: "/page" }),
176180
params: {},
181+
unstable_pattern: "/page",
177182
});
178183
expect(spy).toHaveBeenCalledTimes(1);
179184
expect(getHtml(container)).toContain("Error");
@@ -205,6 +210,7 @@ describe(`handleError`, () => {
205210
expect(spy).toHaveBeenCalledWith(new Error("loader error!"), {
206211
location: expect.objectContaining({ pathname: "/page" }),
207212
params: {},
213+
unstable_pattern: "/page",
208214
});
209215
expect(spy).toHaveBeenCalledTimes(1);
210216
expect(getHtml(container)).toContain("Error");
@@ -241,6 +247,7 @@ describe(`handleError`, () => {
241247
expect(spy).toHaveBeenCalledWith(new Error("action error!"), {
242248
location: expect.objectContaining({ pathname: "/page" }),
243249
params: {},
250+
unstable_pattern: "/page",
244251
});
245252
expect(spy).toHaveBeenCalledTimes(1);
246253
expect(getHtml(container)).toContain("Error");
@@ -270,6 +277,7 @@ describe(`handleError`, () => {
270277
expect(spy).toHaveBeenCalledWith(new Error("loader error!"), {
271278
location: expect.objectContaining({ pathname: "/" }),
272279
params: {},
280+
unstable_pattern: "/",
273281
});
274282
expect(spy).toHaveBeenCalledTimes(1);
275283
expect(getHtml(container)).toContain("Error");
@@ -304,6 +312,7 @@ describe(`handleError`, () => {
304312
expect(spy).toHaveBeenCalledWith(new Error("action error!"), {
305313
location: expect.objectContaining({ pathname: "/" }),
306314
params: {},
315+
unstable_pattern: "/",
307316
});
308317
expect(spy).toHaveBeenCalledTimes(1);
309318
expect(getHtml(container)).toContain("Error");
@@ -334,6 +343,7 @@ describe(`handleError`, () => {
334343
expect(spy).toHaveBeenCalledWith(new Error("render error!"), {
335344
location: expect.objectContaining({ pathname: "/page" }),
336345
params: {},
346+
unstable_pattern: "/page",
337347
errorInfo: expect.objectContaining({
338348
componentStack: expect.any(String),
339349
}),
@@ -379,6 +389,7 @@ describe(`handleError`, () => {
379389
expect(spy).toHaveBeenCalledWith(new Error("await error!"), {
380390
location: expect.objectContaining({ pathname: "/page" }),
381391
params: {},
392+
unstable_pattern: "/page",
382393
});
383394
expect(spy).toHaveBeenCalledTimes(1);
384395
expect(getHtml(container)).toContain("Await Error");
@@ -427,6 +438,7 @@ describe(`handleError`, () => {
427438
expect(spy).toHaveBeenCalledWith(new Error("await error!"), {
428439
location: expect.objectContaining({ pathname: "/page" }),
429440
params: {},
441+
unstable_pattern: "/page",
430442
errorInfo: expect.objectContaining({
431443
componentStack: expect.any(String),
432444
}),
@@ -481,10 +493,12 @@ describe(`handleError`, () => {
481493
expect(spy).toHaveBeenCalledWith(new Error("await error!"), {
482494
location: expect.objectContaining({ pathname: "/page" }),
483495
params: {},
496+
unstable_pattern: "/page",
484497
});
485498
expect(spy).toHaveBeenCalledWith(new Error("errorElement error!"), {
486499
location: expect.objectContaining({ pathname: "/page" }),
487500
params: {},
501+
unstable_pattern: "/page",
488502
errorInfo: expect.objectContaining({
489503
componentStack: expect.any(String),
490504
}),
@@ -534,6 +548,7 @@ describe(`handleError`, () => {
534548
expect(spy).toHaveBeenCalledWith(new Error("loader error!"), {
535549
location: expect.objectContaining({ pathname: "/page" }),
536550
params: {},
551+
unstable_pattern: "/page",
537552
});
538553
expect(spy).toHaveBeenCalledTimes(1);
539554
expect(getHtml(container)).toContain("Error");
@@ -584,6 +599,7 @@ describe(`handleError`, () => {
584599
expect(spy).toHaveBeenCalledWith(new Error("render error!"), {
585600
location: expect.objectContaining({ pathname: "/page" }),
586601
params: {},
602+
unstable_pattern: "/page",
587603
errorInfo: expect.objectContaining({
588604
componentStack: expect.any(String),
589605
}),

‎packages/react-router/lib/components.tsx‎

Lines changed: 29 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,12 @@ import type {
2929
Params,
3030
TrackedPromise,
3131
} from "./router/utils";
32-
import { getResolveToMatches, resolveTo, stripBasename } from "./router/utils";
32+
import {
33+
getResolveToMatches,
34+
getRoutePattern,
35+
resolveTo,
36+
stripBasename,
37+
} from "./router/utils";
3338

3439
import type {
3540
DataRouteObject,
@@ -313,6 +318,7 @@ export interface unstable_ClientOnErrorFunction {
313318
info: {
314319
location: Location;
315320
params: Params;
321+
unstable_pattern: string;
316322
errorInfo?: React.ErrorInfo;
317323
},
318324
): void;
@@ -542,31 +548,23 @@ export function RouterProvider({
542548
nextLocation: Location;
543549
}>();
544550
let fetcherData = React.useRef<Map<string, any>>(new Map());
545-
let logErrorsAndSetState = React.useCallback(
546-
(newState: RouterState) => {
547-
setStateImpl((prevState) => {
548-
// Send loader/action errors through handleError
549-
if (newState.errors && unstable_onError) {
550-
Object.entries(newState.errors).forEach(([routeId, error]) => {
551-
if (prevState.errors?.[routeId] !== error) {
552-
unstable_onError(error, {
553-
location: newState.location,
554-
params: newState.matches[0]?.params ?? {},
555-
});
556-
}
557-
});
558-
}
559-
return newState;
560-
});
561-
},
562-
[unstable_onError],
563-
);
564551

565552
let setState = React.useCallback<RouterSubscriber>(
566553
(
567554
newState: RouterState,
568-
{ deletedFetchers, flushSync, viewTransitionOpts },
555+
{ deletedFetchers, newErrors, flushSync, viewTransitionOpts },
569556
) => {
557+
// Send router errors through onError
558+
if (newErrors && unstable_onError) {
559+
Object.values(newErrors).forEach((error) =>
560+
unstable_onError(error, {
561+
location: newState.location,
562+
params: newState.matches[0]?.params ?? {},
563+
unstable_pattern: getRoutePattern(newState.matches),
564+
}),
565+
);
566+
}
567+
570568
newState.fetchers.forEach((fetcher, key) => {
571569
if (fetcher.data !== undefined) {
572570
fetcherData.current.set(key, fetcher.data);
@@ -600,9 +598,9 @@ export function RouterProvider({
600598
// just update and be done with it
601599
if (!viewTransitionOpts || !isViewTransitionAvailable) {
602600
if (reactDomFlushSyncImpl && flushSync) {
603-
reactDomFlushSyncImpl(() => logErrorsAndSetState(newState));
601+
reactDomFlushSyncImpl(() => setStateImpl(newState));
604602
} else {
605-
React.startTransition(() => logErrorsAndSetState(newState));
603+
React.startTransition(() => setStateImpl(newState));
606604
}
607605
return;
608606
}
@@ -613,7 +611,7 @@ export function RouterProvider({
613611
reactDomFlushSyncImpl(() => {
614612
// Cancel any pending transitions
615613
if (transition) {
616-
renderDfd && renderDfd.resolve();
614+
renderDfd?.resolve();
617615
transition.skipTransition();
618616
}
619617
setVtContext({
@@ -626,7 +624,7 @@ export function RouterProvider({
626624

627625
// Update the DOM
628626
let t = router.window!.document.startViewTransition(() => {
629-
reactDomFlushSyncImpl(() => logErrorsAndSetState(newState));
627+
reactDomFlushSyncImpl(() => setStateImpl(newState));
630628
});
631629

632630
// Clean up after the animation completes
@@ -647,7 +645,7 @@ export function RouterProvider({
647645
if (transition) {
648646
// Interrupting an in-progress transition, cancel and let everything flush
649647
// out, and then kick off a new transition from the interruption state
650-
renderDfd && renderDfd.resolve();
648+
renderDfd?.resolve();
651649
transition.skipTransition();
652650
setInterruption({
653651
state: newState,
@@ -670,7 +668,7 @@ export function RouterProvider({
670668
reactDomFlushSyncImpl,
671669
transition,
672670
renderDfd,
673-
logErrorsAndSetState,
671+
unstable_onError,
674672
],
675673
);
676674

@@ -694,7 +692,7 @@ export function RouterProvider({
694692
let newState = pendingState;
695693
let renderPromise = renderDfd.promise;
696694
let transition = router.window.document.startViewTransition(async () => {
697-
React.startTransition(() => logErrorsAndSetState(newState));
695+
React.startTransition(() => setStateImpl(newState));
698696
await renderPromise;
699697
});
700698
transition.finished.finally(() => {
@@ -705,7 +703,7 @@ export function RouterProvider({
705703
});
706704
setTransition(transition);
707705
}
708-
}, [pendingState, renderDfd, router.window, logErrorsAndSetState]);
706+
}, [pendingState, renderDfd, router.window]);
709707

710708
// When the new location finally renders and is committed to the DOM, this
711709
// effect will run to resolve the transition
@@ -1619,7 +1617,8 @@ export function Await<Resolve>({
16191617
) {
16201618
dataRouterContext.unstable_onError(error, {
16211619
location: dataRouterStateContext.location,
1622-
params: dataRouterStateContext.matches?.[0]?.params || {},
1620+
params: dataRouterStateContext.matches[0]?.params || {},
1621+
unstable_pattern: getRoutePattern(dataRouterStateContext.matches),
16231622
errorInfo,
16241623
});
16251624
}

‎packages/react-router/lib/hooks.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ import {
4343
convertRouteMatchToUiMatch,
4444
decodePath,
4545
getResolveToMatches,
46+
getRoutePattern,
4647
isRouteErrorResponse,
4748
joinPaths,
4849
matchPath,
@@ -1194,6 +1195,7 @@ export function _renderMatches(
11941195
unstable_onError(error, {
11951196
location: dataRouterState.location,
11961197
params: dataRouterState.matches?.[0]?.params ?? {},
1198+
unstable_pattern: getRoutePattern(dataRouterState.matches),
11971199
errorInfo,
11981200
});
11991201
}

‎packages/react-router/lib/router/router.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,7 @@ export interface RouterSubscriber {
489489
state: RouterState,
490490
opts: {
491491
deletedFetchers: string[];
492+
newErrors: RouteData | null;
492493
viewTransitionOpts?: ViewTransitionOpts;
493494
flushSync: boolean;
494495
},
@@ -1292,6 +1293,7 @@ export function createRouter(init: RouterInit): Router {
12921293
[...subscribers].forEach((subscriber) =>
12931294
subscriber(state, {
12941295
deletedFetchers: unmountedFetchers,
1296+
newErrors: newState.errors ?? null,
12951297
viewTransitionOpts: opts.viewTransitionOpts,
12961298
flushSync: opts.flushSync === true,
12971299
}),
@@ -3766,7 +3768,7 @@ export function createStaticHandler(
37663768
let response = await runServerMiddlewarePipeline(
37673769
{
37683770
request,
3769-
unstable_pattern: getRoutePattern(matches.map((m) => m.route.path)),
3771+
unstable_pattern: getRoutePattern(matches),
37703772
matches,
37713773
params: matches[0].params,
37723774
// If we're calling middleware then it must be enabled so we can cast
@@ -3998,7 +4000,7 @@ export function createStaticHandler(
39984000
let response = await runServerMiddlewarePipeline(
39994001
{
40004002
request,
4001-
unstable_pattern: getRoutePattern(matches.map((m) => m.route.path)),
4003+
unstable_pattern: getRoutePattern(matches),
40024004
matches,
40034005
params: matches[0].params,
40044006
// If we're calling middleware then it must be enabled so we can cast
@@ -4391,7 +4393,7 @@ export function createStaticHandler(
43914393
matches.findIndex((m) => m.route.id === pendingActionResult[0]) - 1
43924394
: undefined;
43934395

4394-
let pattern = getRoutePattern(matches.map((m) => m.route.path));
4396+
let pattern = getRoutePattern(matches);
43954397
dsMatches = matches.map((match, index) => {
43964398
if (maxIdx != null && index > maxIdx) {
43974399
return getDataStrategyMatch(
@@ -4865,7 +4867,7 @@ function getMatchesToLoad(
48654867
actionStatus,
48664868
};
48674869

4868-
let pattern = getRoutePattern(matches.map((m) => m.route.path));
4870+
let pattern = getRoutePattern(matches);
48694871
let dsMatches: DataStrategyMatch[] = matches.map((match, index) => {
48704872
let { route } = match;
48714873

@@ -5932,7 +5934,7 @@ function getTargetedDataStrategyMatches(
59325934
mapRouteProperties,
59335935
manifest,
59345936
request,
5935-
getRoutePattern(matches.map((m) => m.route.path)),
5937+
getRoutePattern(matches),
59365938
match,
59375939
lazyRoutePropertiesToSkip,
59385940
scopedContext,
@@ -5960,7 +5962,7 @@ async function callDataStrategyImpl(
59605962
// back out below.
59615963
let dataStrategyArgs = {
59625964
request,
5963-
unstable_pattern: getRoutePattern(matches.map((m) => m.route.path)),
5965+
unstable_pattern: getRoutePattern(matches),
59645966
params: matches[0].params,
59655967
context: scopedContext,
59665968
matches,

‎packages/react-router/lib/router/utils.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2049,6 +2049,12 @@ by the star-slash in the `getRoutePattern` regex and messes up the parsed commen
20492049
for `isRouteErrorResponse` above. This comment seems to reset the parser.
20502050
*/
20512051

2052-
export function getRoutePattern(paths: (string | undefined)[]) {
2053-
return paths.filter(Boolean).join("/").replace(/\/\/*/g, "/") || "/";
2052+
export function getRoutePattern(matches: AgnosticRouteMatch[]) {
2053+
return (
2054+
matches
2055+
.map((m) => m.route.path)
2056+
.filter(Boolean)
2057+
.join("/")
2058+
.replace(/\/\/*/g, "/") || "/"
2059+
);
20542060
}

0 commit comments

Comments
 (0)