feat(remix-server-runtime): RRR 1.3 / 1.4 - handleDocumentRequest

.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)

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", () => {
                 <Form method="post">
                   <button formAction="${HAS_BOUNDARY_ACTION}" type="submit" />
                   <button formAction="${NO_BOUNDARY_ACTION}" type="submit" />
-                  <button formAction="${HAS_BOUNDARY_NO_LOADER_OR_ACTION}" type="submit" />
-                  <button formAction="${NO_BOUNDARY_NO_LOADER_OR_ACTION}" type="submit" />
                 </Form>
 
                 <Link to="${HAS_BOUNDARY_LOADER}">
@@ -79,39 +75,6 @@ test.describe("CatchBoundary", () => {
           }
         `,
 
-        "app/routes/fetcher-boundary.jsx": js`
-          import { useFetcher } from "@remix-run/react";
-          export function CatchBoundary() {
-            return <p id="fetcher-boundary">${OWN_BOUNDARY_TEXT}</p>
-          }
-          export default function() {
-            let fetcher = useFetcher();
-
-            return (
-              <div>
-                <fetcher.Form method="post">
-                  <button formAction="${NO_BOUNDARY_NO_LOADER_OR_ACTION}" type="submit" />
-                </fetcher.Form>
-              </div>
-            )
-          }
-        `,
-
-        "app/routes/fetcher-no-boundary.jsx": js`
-          import { useFetcher } from "@remix-run/react";
-          export default function() {
-            let fetcher = useFetcher();
-
-            return (
-              <div>
-                <fetcher.Form method="post">
-                  <button formAction="${NO_BOUNDARY_NO_LOADER_OR_ACTION}" type="submit" />
-                </fetcher.Form>
-              </div>
-            )
-          }
-        `,
-
         [`app/routes${HAS_BOUNDARY_ACTION}.jsx`]: js`
           import { Form } from "@remix-run/react";
           export async function action() {
@@ -147,21 +110,6 @@ test.describe("CatchBoundary", () => {
           }
         `,
 
-        [`app/routes${HAS_BOUNDARY_NO_LOADER_OR_ACTION}.jsx`]: js`
-          export function CatchBoundary() {
-            return <div id="boundary-no-loader-or-action">${OWN_BOUNDARY_TEXT}</div>
-          }
-          export default function Index() {
-            return <div/>
-          }
-        `,
-
-        [`app/routes${NO_BOUNDARY_NO_LOADER_OR_ACTION}.jsx`]: js`
-          export default function Index() {
-            return <div/>
-          }
-        `,
-
         [`app/routes${HAS_BOUNDARY_LOADER}.jsx`]: js`
           export function loader() {
             throw new Response("", { status: 401 })
@@ -250,12 +198,6 @@ test.describe("CatchBoundary", () => {
     await page.waitForSelector("#root-boundary");
   });
 
-  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);
@@ -334,58 +276,6 @@ test.describe("CatchBoundary", () => {
     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("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("uses correct catch boundary on server action errors", async ({
     page,
   }) => {

integration/compiler-test.ts

@@ -364,6 +364,19 @@ test.describe("compiler", () => {
   });
 
   test.describe("serverBareModulesPlugin", () => {
+    let ogConsole: typeof global.console;
+    test.beforeEach(() => {
+      ogConsole = global.console;
+      // @ts-ignore
+      global.console = {
+        log() {},
+        warn() {},
+        error() {},
+      };
+    });
+    test.afterEach(() => {
+      global.console = ogConsole;
+    });
     test("warns when a module isn't installed", async () => {
       let buildOutput: string;
       let buildStdio = new PassThrough();