diff --git a/.changeset/light-avocados-drop.md b/.changeset/light-avocados-drop.md new file mode 100644 index 00000000000..8be5277aa0b --- /dev/null +++ b/.changeset/light-avocados-drop.md @@ -0,0 +1,26 @@ +--- +"@remix-run/server-runtime": patch +--- + +fix: Properly categorize internal framework-thrown error Responses as error boundary errors + +Previously there was some ambiguity around _"thrown Responses go to the `CatchBoundary`"_. +The `CatchBoundary` exists to give the _user_ a place to handle non-happy path code flows +such that they can throw Response instances from _their own code_ and handle them in a +`CatchBoundary`. However, there are a handful of framework-internal errors that make +sense to have a non-500 status code, and the fact that these were being thrown as Responses +was causing them to go into the CatchBoundary, even though they were not user-thrown. + +With this change, anything thrown by the framework itself (`Error` or `Response`) will +go to the `ErrorBoundary`, and any user-thrown `Response` instances will go to the +`CatchBoundary`. Thereis one exception to this rule, which is that framework-detected +404's will continue to go to the `CatchBoundary` since users should have one single +location to handle 404 displays. + +The primary affected use cases are scenarios such as: + +* HTTP `OPTIONS` requests (405 Unsupported Method ) +* `GET` requests to routes without loaders (400 Bad Request) +* `POST` requests to routes without actions (405 Method Not Allowed) +* Missing route id in `_data` parameters (403 Forbidden) +* Non-matching route id included in `_data` parameters (403 Forbidden) diff --git a/integration/catch-boundary-test.ts b/integration/catch-boundary-test.ts index 46da4bea157..82932111b7c 100644 --- a/integration/catch-boundary-test.ts +++ b/integration/catch-boundary-test.ts @@ -13,10 +13,8 @@ test.describe("CatchBoundary", () => { let HAS_BOUNDARY_LOADER = "/yes/loader"; let HAS_BOUNDARY_ACTION = "/yes/action"; - let HAS_BOUNDARY_NO_LOADER_OR_ACTION = "/yes/no-loader-or-action"; let NO_BOUNDARY_ACTION = "/no/action"; let NO_BOUNDARY_LOADER = "/no/loader"; - let NO_BOUNDARY_NO_LOADER_OR_ACTION = "/no/no-loader-or-action"; let NOT_FOUND_HREF = "/not/found"; @@ -64,8 +62,6 @@ test.describe("CatchBoundary", () => {
+ + @@ -104,7 +112,7 @@ test.describe("ErrorBoundary", () => { throw new Error("Kaboom!") } export function ErrorBoundary() { - return

${OWN_BOUNDARY_TEXT}

+ return

${OWN_BOUNDARY_TEXT}

} export default function () { return ( @@ -138,7 +146,7 @@ test.describe("ErrorBoundary", () => { throw new Error("Kaboom!") } export function ErrorBoundary() { - return
${OWN_BOUNDARY_TEXT}
+ return
${OWN_BOUNDARY_TEXT}
} export default function () { return
@@ -168,7 +176,57 @@ test.describe("ErrorBoundary", () => { } export function ErrorBoundary() { - return
${OWN_BOUNDARY_TEXT}
+ return
${OWN_BOUNDARY_TEXT}
+ } + `, + + [`app/routes${HAS_BOUNDARY_NO_LOADER_OR_ACTION}.jsx`]: js` + export function ErrorBoundary() { + return
${OWN_BOUNDARY_TEXT}
+ } + export default function Index() { + return
+ } + `, + + [`app/routes${NO_BOUNDARY_NO_LOADER_OR_ACTION}.jsx`]: js` + export default function Index() { + return
+ } + `, + + "app/routes/fetcher-boundary.jsx": js` + import { useFetcher } from "@remix-run/react"; + export function ErrorBoundary() { + return

${OWN_BOUNDARY_TEXT}

+ } + export default function() { + let fetcher = useFetcher(); + + return ( +
+ +
+ ) + } + `, + + "app/routes/fetcher-no-boundary.jsx": js` + import { useFetcher } from "@remix-run/react"; + export default function() { + let fetcher = useFetcher(); + + return ( +
+ + + +
+ ) } `, @@ -228,6 +286,12 @@ test.describe("ErrorBoundary", () => { await appFixture.close(); }); + test("invalid request methods", async () => { + let res = await fixture.requestDocument("/", { method: "OPTIONS" }); + expect(res.status).toBe(405); + expect(await res.text()).toMatch(ROOT_BOUNDARY_TEXT); + }); + test("own boundary, action, document request", async () => { let params = new URLSearchParams(); let res = await fixture.postDocument(HAS_BOUNDARY_ACTION, params); @@ -235,17 +299,13 @@ test.describe("ErrorBoundary", () => { expect(await res.text()).toMatch(OWN_BOUNDARY_TEXT); }); - // FIXME: this is broken, test renders the root boundary logging in `RemixRoute` - // test's because the route module hasn't been loaded, my gut tells me that we - // didn't load the route module but tried to render test's boundary, we need the - // module for that! this will probably fix the twin test over in - // catch-boundary-test - test.skip("own boundary, action, client transition from other route", async ({ + test("own boundary, action, client transition from other route", async ({ page, }) => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/"); await app.clickSubmitButton(HAS_BOUNDARY_ACTION); + await page.waitForSelector("#own-boundary"); expect(await app.getHtml("main")).toMatch(OWN_BOUNDARY_TEXT); }); @@ -255,6 +315,7 @@ test.describe("ErrorBoundary", () => { let app = new PlaywrightFixture(appFixture, page); await app.goto(HAS_BOUNDARY_ACTION); await app.clickSubmitButton(HAS_BOUNDARY_ACTION); + await page.waitForSelector("#own-boundary"); expect(await app.getHtml("main")).toMatch(OWN_BOUNDARY_TEXT); }); @@ -353,9 +414,62 @@ test.describe("ErrorBoundary", () => { expect(await app.getHtml("#child-error")).toMatch("Broken!"); }); + test("renders own boundary in fetcher action submission without action from other routes", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/fetcher-boundary"); + await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION); + await page.waitForSelector("#fetcher-boundary"); + }); + + test("renders root boundary in fetcher action submission without action from other routes", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/fetcher-no-boundary"); + await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION); + await page.waitForSelector("#root-boundary"); + }); + + test("renders root boundary in document POST without action requests", async () => { + let res = await fixture.requestDocument(NO_BOUNDARY_NO_LOADER_OR_ACTION, { + method: "post", + }); + expect(res.status).toBe(405); + expect(await res.text()).toMatch(ROOT_BOUNDARY_TEXT); + }); + + test("renders root boundary in action script transitions without action from other routes", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION); + await page.waitForSelector("#root-boundary"); + }); + + test("renders own boundary in document POST without action requests", async () => { + let res = await fixture.requestDocument(HAS_BOUNDARY_NO_LOADER_OR_ACTION, { + method: "post", + }); + expect(res.status).toBe(405); + expect(await res.text()).toMatch(OWN_BOUNDARY_TEXT); + }); + + test("renders own boundary in action script transitions without action from other routes", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/"); + await app.clickSubmitButton(HAS_BOUNDARY_NO_LOADER_OR_ACTION); + await page.waitForSelector("#boundary-no-loader-or-action"); + }); + test.describe("if no error boundary exists in the app", () => { let NO_ROOT_BOUNDARY_LOADER = "/loader-bad"; let NO_ROOT_BOUNDARY_ACTION = "/action-bad"; + let NO_ROOT_BOUNDARY_LOADER_RETURN = "/loader-no-return"; let NO_ROOT_BOUNDARY_ACTION_RETURN = "/action-no-return"; test.beforeAll(async () => { @@ -364,20 +478,20 @@ test.describe("ErrorBoundary", () => { "app/root.jsx": js` import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; - export default function Root() { - return ( - - - - - - - - - - - ); - } + export default function Root() { + return ( + + + + + + + + + + + ); + } `, "app/routes/index.jsx": js` @@ -387,6 +501,7 @@ test.describe("ErrorBoundary", () => { return (

Home

+ Loader no return