From a2956c7fc10ebad30110746c84897b44f66a4c03 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 5 May 2025 13:02:04 -0400 Subject: [PATCH 1/5] Fix middleware error bubbling scenarios --- .changeset/tough-dancers-own.md | 5 + integration/middleware-test.ts | 107 +++++++++++++++++- .../react-router/lib/dom/ssr/single-fetch.tsx | 20 +++- packages/react-router/lib/router/router.ts | 67 ++++++++--- 4 files changed, 180 insertions(+), 19 deletions(-) create mode 100644 .changeset/tough-dancers-own.md diff --git a/.changeset/tough-dancers-own.md b/.changeset/tough-dancers-own.md new file mode 100644 index 0000000000..a7134c568d --- /dev/null +++ b/.changeset/tough-dancers-own.md @@ -0,0 +1,5 @@ +--- +"react-router": patch +--- + +UNSTABLE: Fix a few bugs with error bubbling in middleware use-cases diff --git a/integration/middleware-test.ts b/integration/middleware-test.ts index d433ea0ac5..b59b01a114 100644 --- a/integration/middleware-test.ts +++ b/integration/middleware-test.ts @@ -2140,7 +2140,7 @@ test.describe("Middleware", () => { "app/routes/_index.tsx": js` import { Link } from 'react-router' export default function Component({ loaderData }) { - return Link + return Link } `, "app/routes/a.tsx": js` @@ -2158,7 +2158,7 @@ test.describe("Middleware", () => { return null; } export default function Component() { - return + return } `, "app/routes/a.b.c.tsx": js` @@ -2192,6 +2192,109 @@ test.describe("Middleware", () => { expect(await page.locator("h1").textContent()).toBe("A Error Boundary"); expect(await page.locator("pre").textContent()).toBe("broken!"); + await app.goto("/"); + await app.clickLink("/a/b/c/d"); + expect(await page.locator("h1").textContent()).toBe("A Error Boundary"); + expect(await page.locator("pre").textContent()).toBe("broken!"); + + appFixture.close(); + }); + + test("bubbles errors on the way down up to the deepest error boundary when loaders aren't revalidating", async ({ + page, + }) => { + let fixture = await createFixture( + { + files: { + "react-router.config.ts": reactRouterConfig({ + middleware: true, + }), + "vite.config.ts": js` + import { defineConfig } from "vite"; + import { reactRouter } from "@react-router/dev/vite"; + + export default defineConfig({ + build: { manifest: true, minify: false }, + plugins: [reactRouter()], + }); + `, + "app/routes/_index.tsx": js` + import { Link } from 'react-router' + export default function Component({ loaderData }) { + return ( + <> + /a/b +
+ /a/b/c/d + + ); + } + `, + "app/routes/a.tsx": js` + import { Outlet } from 'react-router' + export default function Component() { + return + } + export function ErrorBoundary({ error }) { + return <>

A Error Boundary

{error.message}
+ } + `, + "app/routes/a.b.tsx": js` + import { Link, Outlet } from 'react-router' + export function loader() { + return { message: "DATA" }; + } + export default function Component({ loaderData }) { + return ( + <> +

AB: {loaderData.message}

+ /a/b/c/d + + + ); + } + export function shouldRevalidate() { + return false; + } + `, + "app/routes/a.b.c.tsx": js` + import { Outlet } from 'react-router' + export default function Component() { + return + } + export function ErrorBoundary({ error }) { + return <>

C Error Boundary

{error.message}
+ } + `, + "app/routes/a.b.c.d.tsx": js` + import { Outlet } from 'react-router' + export const unstable_middleware = [() => { throw new Error("broken!") }] + export const loader = () => null; + export default function Component() { + return + } + `, + }, + }, + UNSAFE_ServerMode.Development + ); + + let appFixture = await createAppFixture( + fixture, + UNSAFE_ServerMode.Development + ); + + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.clickLink("/a/b"); + await page.waitForSelector("[data-ab]"); + expect(await page.locator("[data-ab]").textContent()).toBe("AB: DATA"); + + await app.clickLink("/a/b/c/d"); + await page.waitForSelector("[data-error-c]"); + expect(await page.locator("h1").textContent()).toBe("C Error Boundary"); + expect(await page.locator("pre").textContent()).toBe("broken!"); + appFixture.close(); }); diff --git a/packages/react-router/lib/dom/ssr/single-fetch.tsx b/packages/react-router/lib/dom/ssr/single-fetch.tsx index 964e8f02f7..62c8d60726 100644 --- a/packages/react-router/lib/dom/ssr/single-fetch.tsx +++ b/packages/react-router/lib/dom/ssr/single-fetch.tsx @@ -466,6 +466,20 @@ async function singleFetchLoaderNavigationStrategy( await resolvePromise; + // Capture any middleware errors from routes that weren't actually fetching + // data, these will be bubbled by the router in `processRouteLoaderData` + let fetchedData = await singleFetchDfd.promise; + if ("routes" in fetchedData) { + Object.entries(fetchedData.routes).forEach(([routeId, result]) => { + if ("error" in result && results[routeId]?.type !== "error") { + results[routeId] = { + type: "error", + result: result.error, + }; + } + }); + } + return results; } @@ -694,12 +708,14 @@ function unwrapSingleFetchResult( } let routeResult = result.routes[routeId]; - if ("error" in routeResult) { + if (routeResult == null) { + throw new Error(`No result found for routeId "${routeId}"`); + } else if ("error" in routeResult) { throw routeResult.error; } else if ("data" in routeResult) { return routeResult.data; } else { - throw new Error(`No response found for routeId "${routeId}"`); + throw new Error(`Invalid response found for routeId "${routeId}"`); } } diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index ee41ea8e79..9377749504 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -3614,28 +3614,50 @@ export function createStaticHandler( dataRoutes, renderedStaticContext, error, - findNearestBoundary(matches!, routeId).route.id + skipLoaderErrorBubbling + ? routeId + : findNearestBoundary(matches, routeId).route.id ) ); } else { // We never even got to the handlers, so we've got no data - // just create an empty context reflecting the error. - // Find the boundary at or above the highest loader. We can't - // render any UI below there since we have no loader data available - let loaderIdx = matches!.findIndex((m) => m.route.loader); - let boundary = - loaderIdx >= 0 - ? findNearestBoundary(matches!, matches![loaderIdx].route.id) - : findNearestBoundary(matches!); + + let boundaryRouteId = skipLoaderErrorBubbling + ? routeId + : // Find the boundary at or above the source of the middleware + // error or the highest loader. We can't render any UI below + // the highest loader since we have no loader data available + findNearestBoundary( + matches, + matches.find( + (m) => m.route.id === routeId || m.route.loader + )?.route.id || routeId + ).route.id; + + // If we errored in the top-down middleware, stub in `undefined` + // for all loaders the front end is expecting results for + let loaderData: RouterState["loaderData"] = {}; + if (!isMutationMethod(request.method)) { + matches + .filter((m) => + filterMatchesToLoad + ? filterMatchesToLoad(m) + : m.route.loader + ) + .forEach((m) => { + loaderData[m.route.id] = undefined; + }); + } return respond({ matches: matches!, location, basename, - loaderData: {}, + loaderData, actionData: null, errors: { - [boundary.route.id]: error, + [boundaryRouteId]: error, }, statusCode: isRouteErrorResponse(error) ? error.status : 500, actionHeaders: {}, @@ -4219,6 +4241,7 @@ export function createStaticHandler( matches, results, pendingActionResult, + undefined, true, skipLoaderErrorBubbling ); @@ -5987,6 +6010,7 @@ function processRouteLoaderData( matches: AgnosticDataRouteMatch[], results: Record, pendingActionResult: PendingActionResult | undefined, + currentLoaderData?: RouterState["loaderData"], isStaticHandler = false, skipLoaderErrorBubbling = false ): { @@ -6032,10 +6056,22 @@ function processRouteLoaderData( if (skipLoaderErrorBubbling) { errors[id] = error; } else { - // Look upwards from the matched route for the closest ancestor error - // boundary, defaulting to the root match. Prefer higher error values - // if lower errors bubble to the same boundary - let boundaryMatch = findNearestBoundary(matches, id); + // Bubble the error to the proper error boundary by looking upwards from + // the highest route that defines a `loader` but doesn't currently have + // any `loaderData`. This situation can happen if a middleware throws + // on the way down and thus a loader never executes for a given route. + // If such a route doesn't exist, then we just look upwards from the + // throwing route. Prefer higher error values if lower errors bubble to + // the same boundary + let highestLoaderRouteWithoutData = currentLoaderData + ? matches.find( + (m) => m.route.loader && !(m.route.id in currentLoaderData) + ) + : undefined; + let boundaryMatch = findNearestBoundary( + matches, + highestLoaderRouteWithoutData?.route.id || id + ); if (errors[boundaryMatch.route.id] == null) { errors[boundaryMatch.route.id] = error; } @@ -6102,7 +6138,8 @@ function processLoaderData( let { loaderData, errors } = processRouteLoaderData( matches, results, - pendingActionResult + pendingActionResult, + state.loaderData ); // Process results from our revalidating fetchers From 148e35e36c9503e9120a8c1ec6684e14cb00a322 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 5 May 2025 17:37:28 -0400 Subject: [PATCH 2/5] Change approach --- .../react-router/lib/dom/ssr/single-fetch.tsx | 35 ++++++++++++++----- packages/react-router/lib/router/router.ts | 34 ++---------------- 2 files changed, 28 insertions(+), 41 deletions(-) diff --git a/packages/react-router/lib/dom/ssr/single-fetch.tsx b/packages/react-router/lib/dom/ssr/single-fetch.tsx index 62c8d60726..3d07a3f463 100644 --- a/packages/react-router/lib/dom/ssr/single-fetch.tsx +++ b/packages/react-router/lib/dom/ssr/single-fetch.tsx @@ -24,6 +24,8 @@ import type { DataRouteMatch } from "../../context"; export const SingleFetchRedirectSymbol = Symbol("SingleFetchRedirect"); +class SingleFetchNoResultError extends Error {} + export type SingleFetchRedirectResult = { redirect: string; status: number; @@ -466,16 +468,29 @@ async function singleFetchLoaderNavigationStrategy( await resolvePromise; - // Capture any middleware errors from routes that weren't actually fetching - // data, these will be bubbled by the router in `processRouteLoaderData` + // If a middleware threw on the way down, we won't have data for our requested + // loaders and they'll resolve to `SingleFetchNoResultError` results. If this + // happens, take the highest error we find in our results (which is a middleware + // error if no loaders ever ran), and assign to these missing routes and let + // the router bubble accordingly + let middlewareError: unknown; let fetchedData = await singleFetchDfd.promise; if ("routes" in fetchedData) { - Object.entries(fetchedData.routes).forEach(([routeId, result]) => { - if ("error" in result && results[routeId]?.type !== "error") { - results[routeId] = { - type: "error", - result: result.error, - }; + for (let match of args.matches) { + if (match.route.id in fetchedData.routes) { + let routeResult = fetchedData.routes[match.route.id]; + if ("error" in routeResult) { + middlewareError = routeResult.error; + break; + } + } + } + } + + if (middlewareError !== undefined) { + Array.from(routesParams.values()).forEach((routeId) => { + if (results[routeId].result instanceof SingleFetchNoResultError) { + results[routeId].result = middlewareError; } }); } @@ -709,7 +724,9 @@ function unwrapSingleFetchResult( let routeResult = result.routes[routeId]; if (routeResult == null) { - throw new Error(`No result found for routeId "${routeId}"`); + throw new SingleFetchNoResultError( + `No result found for routeId "${routeId}"` + ); } else if ("error" in routeResult) { throw routeResult.error; } else if ("data" in routeResult) { diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index 9377749504..fdcbe1d92f 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -3635,26 +3635,11 @@ export function createStaticHandler( )?.route.id || routeId ).route.id; - // If we errored in the top-down middleware, stub in `undefined` - // for all loaders the front end is expecting results for - let loaderData: RouterState["loaderData"] = {}; - if (!isMutationMethod(request.method)) { - matches - .filter((m) => - filterMatchesToLoad - ? filterMatchesToLoad(m) - : m.route.loader - ) - .forEach((m) => { - loaderData[m.route.id] = undefined; - }); - } - return respond({ matches: matches!, location, basename, - loaderData, + loaderData: {}, actionData: null, errors: { [boundaryRouteId]: error, @@ -6056,22 +6041,7 @@ function processRouteLoaderData( if (skipLoaderErrorBubbling) { errors[id] = error; } else { - // Bubble the error to the proper error boundary by looking upwards from - // the highest route that defines a `loader` but doesn't currently have - // any `loaderData`. This situation can happen if a middleware throws - // on the way down and thus a loader never executes for a given route. - // If such a route doesn't exist, then we just look upwards from the - // throwing route. Prefer higher error values if lower errors bubble to - // the same boundary - let highestLoaderRouteWithoutData = currentLoaderData - ? matches.find( - (m) => m.route.loader && !(m.route.id in currentLoaderData) - ) - : undefined; - let boundaryMatch = findNearestBoundary( - matches, - highestLoaderRouteWithoutData?.route.id || id - ); + let boundaryMatch = findNearestBoundary(matches, id); if (errors[boundaryMatch.route.id] == null) { errors[boundaryMatch.route.id] = error; } From b4660246e1d2f8496f9b4c2f2ef7ef0191444842 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 5 May 2025 17:38:29 -0400 Subject: [PATCH 3/5] Remove unused param --- packages/react-router/lib/router/router.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index fdcbe1d92f..32f1dcee12 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -4226,7 +4226,6 @@ export function createStaticHandler( matches, results, pendingActionResult, - undefined, true, skipLoaderErrorBubbling ); @@ -5995,7 +5994,6 @@ function processRouteLoaderData( matches: AgnosticDataRouteMatch[], results: Record, pendingActionResult: PendingActionResult | undefined, - currentLoaderData?: RouterState["loaderData"], isStaticHandler = false, skipLoaderErrorBubbling = false ): { @@ -6108,8 +6106,7 @@ function processLoaderData( let { loaderData, errors } = processRouteLoaderData( matches, results, - pendingActionResult, - state.loaderData + pendingActionResult ); // Process results from our revalidating fetchers From 766c6e614b526bb2f2cfd728b17b844770f9b73b Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 5 May 2025 17:40:40 -0400 Subject: [PATCH 4/5] updates --- packages/react-router/lib/router/router.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/react-router/lib/router/router.ts b/packages/react-router/lib/router/router.ts index 32f1dcee12..3b5c97f9b1 100644 --- a/packages/react-router/lib/router/router.ts +++ b/packages/react-router/lib/router/router.ts @@ -3623,12 +3623,12 @@ export function createStaticHandler( // We never even got to the handlers, so we've got no data - // just create an empty context reflecting the error. + // Find the boundary at or above the source of the middleware + // error or the highest loader. We can't render any UI below + // the highest loader since we have no loader data available let boundaryRouteId = skipLoaderErrorBubbling ? routeId - : // Find the boundary at or above the source of the middleware - // error or the highest loader. We can't render any UI below - // the highest loader since we have no loader data available - findNearestBoundary( + : findNearestBoundary( matches, matches.find( (m) => m.route.id === routeId || m.route.loader @@ -6039,6 +6039,9 @@ function processRouteLoaderData( if (skipLoaderErrorBubbling) { errors[id] = error; } else { + // Look upwards from the matched route for the closest ancestor error + // boundary, defaulting to the root match. Prefer higher error values + // if lower errors bubble to the same boundary let boundaryMatch = findNearestBoundary(matches, id); if (errors[boundaryMatch.route.id] == null) { errors[boundaryMatch.route.id] = error; From 42412184feac87723795dc6a8cf01bb43697e008 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 6 May 2025 12:08:44 -0400 Subject: [PATCH 5/5] Fix e2e test --- .../react-router/lib/dom/ssr/single-fetch.tsx | 66 ++++++++++++------- 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/packages/react-router/lib/dom/ssr/single-fetch.tsx b/packages/react-router/lib/dom/ssr/single-fetch.tsx index 3d07a3f463..63f5ac9fbc 100644 --- a/packages/react-router/lib/dom/ssr/single-fetch.tsx +++ b/packages/react-router/lib/dom/ssr/single-fetch.tsx @@ -468,34 +468,54 @@ async function singleFetchLoaderNavigationStrategy( await resolvePromise; - // If a middleware threw on the way down, we won't have data for our requested - // loaders and they'll resolve to `SingleFetchNoResultError` results. If this - // happens, take the highest error we find in our results (which is a middleware - // error if no loaders ever ran), and assign to these missing routes and let - // the router bubble accordingly - let middlewareError: unknown; - let fetchedData = await singleFetchDfd.promise; - if ("routes" in fetchedData) { - for (let match of args.matches) { - if (match.route.id in fetchedData.routes) { - let routeResult = fetchedData.routes[match.route.id]; - if ("error" in routeResult) { - middlewareError = routeResult.error; - break; + await bubbleMiddlewareErrors( + singleFetchDfd.promise, + args.matches, + routesParams, + results + ); + + return results; +} + +// If a middleware threw on the way down, we won't have data for our requested +// loaders and they'll resolve to `SingleFetchNoResultError` results. If this +// happens, take the highest error we find in our results (which is a middleware +// error if no loaders ever ran), and assign to these missing routes and let +// the router bubble accordingly +async function bubbleMiddlewareErrors( + singleFetchPromise: Promise, + matches: DataStrategyFunctionArgs["matches"], + routesParams: Set, + results: Record +) { + try { + let middlewareError: unknown; + let fetchedData = await singleFetchPromise; + + if ("routes" in fetchedData) { + for (let match of matches) { + if (match.route.id in fetchedData.routes) { + let routeResult = fetchedData.routes[match.route.id]; + if ("error" in routeResult) { + middlewareError = routeResult.error; + break; + } } } } - } - if (middlewareError !== undefined) { - Array.from(routesParams.values()).forEach((routeId) => { - if (results[routeId].result instanceof SingleFetchNoResultError) { - results[routeId].result = middlewareError; - } - }); + if (middlewareError !== undefined) { + Array.from(routesParams.values()).forEach((routeId) => { + if (results[routeId].result instanceof SingleFetchNoResultError) { + results[routeId].result = middlewareError; + } + }); + } + } catch (e) { + // No-op - this logic is only intended to process successful responses + // If the `.data` failed, the routes will handle those errors themselves } - - return results; } // Fetcher loader calls are much simpler than navigational loader calls