Skip to content

Commit 10c8a7e

Browse files
brophdawg11brophdawg11
authored
Fix issue with client pre-next middleware error bubbling (#14150)
1 parent fdd3ab3 commit 10c8a7e

File tree

4 files changed

+64
-33
lines changed

4 files changed

+64
-33
lines changed

‎.changeset/cold-maps-wave.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+
Bubble client pre-next middleware error to the shallowest ancestor that needs to load, not strictly the shallowest ancestor with a loader

‎integration/middleware-test.ts‎

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,7 @@ test.describe("Middleware", () => {
490490
return <h1>Should not see me</h1>
491491
}
492492
export function ErrorBoundary({ error }) {
493-
return <h1>{error.message}</h1>
493+
return <h1 data-error>{error.message}</h1>
494494
}
495495
`,
496496
},
@@ -508,10 +508,8 @@ test.describe("Middleware", () => {
508508
await page.waitForSelector('a:has-text("Link")');
509509

510510
(await page.getByRole("link"))?.click();
511-
// Bubbles to the root error boundary in SPA mode because every route has
512-
// a loader to load the client assets
513-
await page.waitForSelector('h1:has-text("Application Error")');
514-
await page.waitForSelector('pre:has-text("Error: broken!")');
511+
await page.waitForSelector("[data-error]");
512+
expect(await page.innerText("[data-error]")).toBe("broken!");
515513

516514
appFixture.close();
517515
});
@@ -557,7 +555,7 @@ test.describe("Middleware", () => {
557555
export function ErrorBoundary({ loaderData, error }) {
558556
return (
559557
<>
560-
<h1>{error.message}</h1>
558+
<h1 data-error>{error.message}</h1>
561559
<pre>{loaderData ?? 'empty'}</pre>
562560
</>
563561
);
@@ -578,7 +576,8 @@ test.describe("Middleware", () => {
578576
await page.waitForSelector('a:has-text("Link")');
579577

580578
(await page.getByRole("link"))?.click();
581-
await page.waitForSelector('h1:has-text("broken!")');
579+
await page.waitForSelector("[data-error]");
580+
expect(await page.innerText("[data-error]")).toBe("broken!");
582581
expect(await page.innerText("pre")).toBe("empty");
583582

584583
appFixture.close();
@@ -1120,7 +1119,6 @@ test.describe("Middleware", () => {
11201119
}
11211120
`,
11221121
"app/routes/broken.tsx": js`
1123-
import { useRouteError } from 'react-router'
11241122
export const unstable_clientMiddleware = [
11251123
async ({ request, context }, next) => {
11261124
throw new Error('broken!')
@@ -1129,8 +1127,8 @@ test.describe("Middleware", () => {
11291127
export default function Component() {
11301128
return <h1>Should not see me</h1>
11311129
}
1132-
export function ErrorBoundary() {
1133-
return <h1>{useRouteError().message}</h1>
1130+
export function ErrorBoundary({ error }) {
1131+
return <h1 data-error>{error.message}</h1>
11341132
}
11351133
`,
11361134
},
@@ -1148,10 +1146,8 @@ test.describe("Middleware", () => {
11481146
await page.waitForSelector('a:has-text("Link")');
11491147

11501148
(await page.getByRole("link"))?.click();
1151-
// Bubbles to the root error boundary in SPA mode because every route has
1152-
// a loader to load the client assets
1153-
await page.waitForSelector('h1:has-text("Application Error")');
1154-
await page.waitForSelector('pre:has-text("Error: broken!")');
1149+
await page.waitForSelector("[data-error]");
1150+
expect(await page.innerText("[data-error]")).toBe("broken!");
11551151

11561152
appFixture.close();
11571153
});
@@ -1196,7 +1192,7 @@ test.describe("Middleware", () => {
11961192
export function ErrorBoundary({ loaderData, error }) {
11971193
return (
11981194
<>
1199-
<h1>{error.message}</h1>
1195+
<h1 data-error>{error.message}</h1>
12001196
<pre>{loaderData ?? 'empty'}</pre>
12011197
</>
12021198
);
@@ -1217,7 +1213,8 @@ test.describe("Middleware", () => {
12171213
await page.waitForSelector('a:has-text("Link")');
12181214

12191215
(await page.getByRole("link"))?.click();
1220-
await page.waitForSelector('h1:has-text("broken!")');
1216+
await page.waitForSelector("[data-error]");
1217+
expect(await page.innerText("[data-error]")).toBe("broken!");
12211218
expect(await page.innerText("pre")).toBe("empty");
12221219

12231220
appFixture.close();
@@ -1987,7 +1984,7 @@ test.describe("Middleware", () => {
19871984
return <h1>Should not see me</h1>
19881985
}
19891986
export function ErrorBoundary({ error }) {
1990-
return <h1>{error.message}</h1>
1987+
return <h1 data-error>{error.message}</h1>
19911988
}
19921989
`,
19931990
},
@@ -2002,7 +1999,7 @@ test.describe("Middleware", () => {
20021999

20032000
let app = new PlaywrightFixture(appFixture, page);
20042001
await app.goto("/broken");
2005-
expect(await page.innerText("h1")).toBe("broken!");
2002+
expect(await page.innerText("[data-error]")).toBe("broken!");
20062003
expect(errors).toEqual([
20072004
["handleError", "GET", "/broken", new Error("broken!")],
20082005
]);
@@ -2049,7 +2046,7 @@ test.describe("Middleware", () => {
20492046
return <h1>Should not see me</h1>
20502047
}
20512048
export function ErrorBoundary({ error }) {
2052-
return <h1>{error.message}</h1>
2049+
return <h1 data-error>{error.message}</h1>
20532050
}
20542051
`,
20552052
},
@@ -2066,8 +2063,8 @@ test.describe("Middleware", () => {
20662063
await app.goto("/");
20672064

20682065
(await page.$('a[href="/broken"]'))?.click();
2069-
await page.waitForSelector("h1");
2070-
expect(await page.innerText("h1")).toBe("broken!");
2066+
await page.waitForSelector("[data-error]");
2067+
expect(await page.innerText("[data-error]")).toBe("broken!");
20712068
expect(errors).toEqual([
20722069
["handleError", "GET", "/broken.data", new Error("broken!")],
20732070
]);
@@ -2117,7 +2114,7 @@ test.describe("Middleware", () => {
21172114
export function ErrorBoundary({ error, loaderData }) {
21182115
return (
21192116
<>
2120-
<h1>{error.message}</h1>
2117+
<h1 data-error>{error.message}</h1>
21212118
<pre>{loaderData ?? 'empty'}</pre>
21222119
</>
21232120
);
@@ -2135,7 +2132,7 @@ test.describe("Middleware", () => {
21352132

21362133
let app = new PlaywrightFixture(appFixture, page);
21372134
await app.goto("/broken");
2138-
expect(await page.innerText("h1")).toBe("broken!");
2135+
expect(await page.innerText("[data-error]")).toBe("broken!");
21392136
expect(await page.innerText("pre")).toBe("empty");
21402137
expect(errors).toEqual([
21412138
["handleError", "GET", "/broken", new Error("broken!")],
@@ -2186,7 +2183,7 @@ test.describe("Middleware", () => {
21862183
export function ErrorBoundary({ error, loaderData }) {
21872184
return (
21882185
<>
2189-
<h1>{error.message}</h1>
2186+
<h1 data-error>{error.message}</h1>
21902187
<pre>{loaderData ?? 'empty'}</pre>
21912188
</>
21922189
);
@@ -2207,7 +2204,8 @@ test.describe("Middleware", () => {
22072204

22082205
(await page.$('a[href="/broken"]'))?.click();
22092206
await page.waitForSelector("h1");
2210-
expect(await page.innerText("h1")).toBe("broken!");
2207+
await page.waitForSelector("[data-error]");
2208+
expect(await page.innerText("[data-error]")).toBe("broken!");
22112209
expect(await page.innerText("pre")).toBe("empty");
22122210
expect(errors).toEqual([
22132211
["handleError", "GET", "/broken.data", new Error("broken!")],

‎packages/react-router/__tests__/router/context-middleware-test.ts‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1409,6 +1409,17 @@ describe("context/middleware", () => {
14091409
],
14101410
loader: () => "D",
14111411
},
1412+
{
1413+
id: "e",
1414+
path: "e",
1415+
hasErrorBoundary: true,
1416+
unstable_middleware: [
1417+
() => {
1418+
throw new Error("E ERROR");
1419+
},
1420+
],
1421+
loader: () => "E",
1422+
},
14121423
],
14131424
},
14141425
],
@@ -1418,12 +1429,24 @@ describe("context/middleware", () => {
14181429
],
14191430
});
14201431

1432+
// Bubbles to B because it's the initial load and it's loader hasn't run
14211433
await router.navigate("/a/b/c/d");
1422-
14231434
expect(router.state.loaderData).toEqual({});
14241435
expect(router.state.errors).toEqual({
14251436
b: new Error("D ERROR"),
14261437
});
1438+
1439+
// Load data into B
1440+
await router.navigate("/a/b");
1441+
expect(router.state.loaderData).toEqual({ b: "B" });
1442+
expect(router.state.errors).toEqual(null);
1443+
1444+
// B doesn't have to revalidate so we can surface this error at E
1445+
await router.navigate("/a/b/c/e");
1446+
expect(router.state.loaderData).toEqual({ b: "B" });
1447+
expect(router.state.errors).toEqual({
1448+
e: new Error("E ERROR"),
1449+
});
14271450
});
14281451
});
14291452
});

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5389,22 +5389,27 @@ async function defaultDataStrategyWithMiddleware(
53895389
function clientMiddlewareErrorHandler(
53905390
error: unknown,
53915391
routeId: string,
5392-
matches: AgnosticDataRouteMatch[],
5392+
matches: DataStrategyMatch[],
53935393
didCallHandler: boolean,
53945394
): Record<string, DataStrategyResult> {
53955395
if (didCallHandler) {
53965396
return {
53975397
[routeId]: { type: "error", result: error },
53985398
};
53995399
} else {
5400-
// We never even got to the handlers, so we've got no data.
5401-
// Find the boundary at or above the source of the middleware
5402-
// error or the highest loader. We can't render any UI below
5403-
// the highest loader since we have no loader data available
5400+
// We never even got to the handlers, so we might not have data for new routes.
5401+
// Find the boundary at or above the source of the middleware error or the
5402+
// highest route that needs to load - we can't render any UI below that since
5403+
// we won't have valid loader data.
5404+
let maxBoundaryIdx = Math.min(
5405+
// Throwing route
5406+
matches.findIndex((m) => m.route.id === routeId) || 0,
5407+
// or the shallowest route that needs to load data
5408+
matches.findIndex((m) => m.unstable_shouldCallHandler()) || 0,
5409+
);
54045410
let boundaryRouteId = findNearestBoundary(
54055411
matches,
5406-
matches.find((m) => m.route.id === routeId || m.route.loader)?.route.id ||
5407-
routeId,
5412+
matches[maxBoundaryIdx].route.id,
54085413
).route.id;
54095414
return {
54105415
[boundaryRouteId]: { type: "error", result: error },

0 commit comments

Comments
 (0)