fix(remix-dev): update handleError to properly receive `ErrorRespon…

…se` instances (#7211)

.changeset/handle-error-response.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/server-runtime": patch
+---
+
+Fix `handleError` method to correctly receive `ErrorResponse` instances on `?_data` and resource route requests. It now receives the `ErrorResponse` instance the same way a document request would.  Users can leverage `isRouteErrorResponse`to detect these error instances and log accordingly.

integration/error-data-request-test.ts

@@ -11,13 +11,11 @@ test.describe("ErrorBoundary", () => {
   let fixture: Fixture;
   let appFixture: AppFixture;
   let _consoleError: any;
-  let errorLogs: string[];
+  let errorLogs: any[];
 
   test.beforeAll(async () => {
     _consoleError = console.error;
-    console.error = (whatever) => {
-      errorLogs.push(String(whatever));
-    };
+    console.error = (v) => errorLogs.push(v);
 
     fixture = await createFixture({
       files: {
@@ -105,17 +103,20 @@ test.describe("ErrorBoundary", () => {
     appFixture.close();
   });
 
-  function assertConsoleError(str: string) {
-    expect(errorLogs[0]).toEqual(str);
+  function assertLoggedErrorInstance(message: string) {
+    let error = errorLogs[0] as Error;
+    expect(error).toBeInstanceOf(Error);
+    expect(error.message).toEqual(message);
   }
 
   test("returns a 400 x-remix-error on a data fetch to a path with no loader", async () => {
     let response = await fixture.requestData("/", "routes/_index");
     expect(response.status).toBe(400);
     expect(response.headers.get("X-Remix-Error")).toBe("yes");
     expect(await response.text()).toMatch("Unexpected Server Error");
-    assertConsoleError(
-      'Error: You made a GET request to "/" but did not provide a `loader` for route "routes/_index", so there is no way to handle the request.'
+    expect(errorLogs[0]).toBeInstanceOf(Error);
+    assertLoggedErrorInstance(
+      'You made a GET request to "/" but did not provide a `loader` for route "routes/_index", so there is no way to handle the request.'
     );
   });
 
@@ -126,8 +127,8 @@ test.describe("ErrorBoundary", () => {
     expect(response.status).toBe(405);
     expect(response.headers.get("X-Remix-Error")).toBe("yes");
     expect(await response.text()).toMatch("Unexpected Server Error");
-    assertConsoleError(
-      'Error: You made a POST request to "/" but did not provide an `action` for route "routes/_index", so there is no way to handle the request.'
+    assertLoggedErrorInstance(
+      'You made a POST request to "/" but did not provide an `action` for route "routes/_index", so there is no way to handle the request.'
     );
   });
 
@@ -147,8 +148,8 @@ test.describe("ErrorBoundary", () => {
     expect(response.status).toBe(403);
     expect(response.headers.get("X-Remix-Error")).toBe("yes");
     expect(await response.text()).toMatch("Unexpected Server Error");
-    assertConsoleError(
-      'Error: Route "routes/loader-return-json" does not match URL "/"'
+    assertLoggedErrorInstance(
+      'Route "routes/loader-return-json" does not match URL "/"'
     );
   });
 
@@ -159,8 +160,8 @@ test.describe("ErrorBoundary", () => {
     expect(response.status).toBe(403);
     expect(response.headers.get("X-Remix-Error")).toBe("yes");
     expect(await response.text()).toMatch("Unexpected Server Error");
-    assertConsoleError(
-      'Error: Route "routes/loader-return-json" does not match URL "/"'
+    assertLoggedErrorInstance(
+      'Route "routes/loader-return-json" does not match URL "/"'
     );
   });
 
@@ -169,6 +170,6 @@ test.describe("ErrorBoundary", () => {
     expect(response.status).toBe(404);
     expect(response.headers.get("X-Remix-Error")).toBe("yes");
     expect(await response.text()).toMatch("Unexpected Server Error");
-    assertConsoleError('Error: No route matches URL "/i/match/nothing"');
+    assertLoggedErrorInstance('No route matches URL "/i/match/nothing"');
   });
 });

integration/error-sanitization-test.ts

@@ -500,7 +500,11 @@ test.describe("Error Sanitization", () => {
               ) {
                 console.error("App Specific Error Logging:");
                 console.error("  Request: " + request.method + " " + request.url);
-                if (error instanceof Error) {
+                if (isRouteErrorResponse(error)) {
+                  console.error("  Status: " + error.status + " " + error.statusText);
+                  console.error("  Error: " + error.error.message);
+                  console.error("  Stack: " + error.error.stack);
+                } else if (error instanceof Error) {
                   console.error("  Error: " + error.message);
                   console.error("  Stack: " + error.stack);
                 } else {
@@ -647,11 +651,12 @@ test.describe("Error Sanitization", () => {
       expect(errorLogs[1][0]).toEqual(
         "  Request: GET test://test/?_data=not-a-route"
       );
-      expect(errorLogs[2][0]).toEqual(
+      expect(errorLogs[2][0]).toEqual("  Status: 403 Forbidden");
+      expect(errorLogs[3][0]).toEqual(
         '  Error: Route "not-a-route" does not match URL "/"'
       );
-      expect(errorLogs[3][0]).toMatch(" at ");
-      expect(errorLogs.length).toBe(4);
+      expect(errorLogs[4][0]).toMatch(" at ");
+      expect(errorLogs.length).toBe(5);
     });
   });
 });

packages/remix-server-runtime/__tests__/handle-error-test.ts

@@ -0,0 +1,226 @@
+import { ErrorResponse } from "@remix-run/router";
+
+import type { ServerBuild } from "../build";
+import { createRequestHandler } from "../server";
+import { json } from "../responses";
+
+function getHandler(routeModule = {}, entryServerModule = {}) {
+  let routeId = "root";
+  let handleErrorSpy = jest.fn();
+  let build = {
+    routes: {
+      [routeId]: {
+        id: routeId,
+        path: "/",
+        module: {
+          default() {},
+          ...routeModule,
+        },
+      },
+    },
+    entry: {
+      module: {
+        handleError: handleErrorSpy,
+        default() {},
+        ...entryServerModule,
+      },
+    },
+  } as unknown as ServerBuild;
+
+  return {
+    handler: createRequestHandler(build),
+    handleErrorSpy,
+  };
+}
+
+describe("handleError", () => {
+  describe("document request", () => {
+    it("provides user-thrown Error", async () => {
+      let error = new Error("💥");
+      let { handler, handleErrorSpy } = getHandler({
+        loader() {
+          throw error;
+        },
+      });
+      let request = new Request("http://example.com/");
+      await handler(request);
+      expect(handleErrorSpy).toHaveBeenCalledWith(error, {
+        request,
+        params: {},
+        context: {},
+      });
+    });
+
+    it("provides router-thrown ErrorResponse", async () => {
+      let { handler, handleErrorSpy } = getHandler({});
+      let request = new Request("http://example.com/", { method: "post" });
+      await handler(request);
+      expect(handleErrorSpy).toHaveBeenCalledWith(
+        new ErrorResponse(
+          405,
+          "Method Not Allowed",
+          new Error(
+            'You made a POST request to "/" but did not provide an `action` for route "root", so there is no way to handle the request.'
+          ),
+          true
+        ),
+        {
+          request,
+          params: {},
+          context: {},
+        }
+      );
+    });
+
+    it("provides render-thrown Error", async () => {
+      let { handler, handleErrorSpy } = getHandler(undefined, {
+        default() {
+          throw new Error("Render error");
+        },
+      });
+      let request = new Request("http://example.com/");
+      await handler(request);
+      expect(handleErrorSpy).toHaveBeenCalledWith(new Error("Render error"), {
+        request,
+        params: {},
+        context: {},
+      });
+    });
+
+    it("does not provide user-thrown Responses to handleError", async () => {
+      let { handler, handleErrorSpy } = getHandler({
+        loader() {
+          throw json(
+            { message: "not found" },
+            { status: 404, statusText: "Not Found" }
+          );
+        },
+      });
+      let request = new Request("http://example.com/");
+      await handler(request);
+      expect(handleErrorSpy).not.toHaveBeenCalled();
+    });
+  });
+
+  describe("data request", () => {
+    it("provides user-thrown Error", async () => {
+      let error = new Error("💥");
+      let { handler, handleErrorSpy } = getHandler({
+        loader() {
+          throw error;
+        },
+      });
+      let request = new Request("http://example.com/?_data=root");
+      await handler(request);
+      expect(handleErrorSpy).toHaveBeenCalledWith(error, {
+        request,
+        params: {},
+        context: {},
+      });
+    });
+
+    it("provides router-thrown ErrorResponse", async () => {
+      let { handler, handleErrorSpy } = getHandler({});
+      let request = new Request("http://example.com/?_data=root", {
+        method: "post",
+      });
+      await handler(request);
+      expect(handleErrorSpy).toHaveBeenCalledWith(
+        new ErrorResponse(
+          405,
+          "Method Not Allowed",
+          new Error(
+            'You made a POST request to "/" but did not provide an `action` for route "root", so there is no way to handle the request.'
+          ),
+          true
+        ),
+        {
+          request,
+          params: {},
+          context: {},
+        }
+      );
+    });
+
+    it("does not provide user-thrown Responses to handleError", async () => {
+      let { handler, handleErrorSpy } = getHandler({
+        loader() {
+          throw json(
+            { message: "not found" },
+            { status: 404, statusText: "Not Found" }
+          );
+        },
+      });
+      let request = new Request("http://example.com/?_data=root");
+      await handler(request);
+      expect(handleErrorSpy).not.toHaveBeenCalled();
+    });
+  });
+
+  describe("resource request", () => {
+    it("provides user-thrown Error", async () => {
+      let error = new Error("💥");
+      let { handler, handleErrorSpy } = getHandler({
+        loader() {
+          throw error;
+        },
+        default: null,
+      });
+      let request = new Request("http://example.com/");
+      await handler(request);
+      expect(handleErrorSpy).toHaveBeenCalledWith(error, {
+        request,
+        params: {},
+        context: {},
+      });
+    });
+
+    it("provides router-thrown ErrorResponse", async () => {
+      let { handler, handleErrorSpy } = getHandler({ default: null });
+      let request = new Request("http://example.com/", {
+        method: "post",
+      });
+      await handler(request);
+      expect(handleErrorSpy).toHaveBeenCalledWith(
+        new ErrorResponse(
+          405,
+          "Method Not Allowed",
+          new Error(
+            'You made a POST request to "/" but did not provide an `action` for route "root", so there is no way to handle the request.'
+          ),
+          true
+        ),
+        {
+          request,
+          params: {},
+          context: {},
+        }
+      );
+    });
+
+    it("does not provide user-thrown Responses to handleError", async () => {
+      let { handler, handleErrorSpy } = getHandler({
+        loader() {
+          throw json(
+            { message: "not found" },
+            { status: 404, statusText: "Not Found" }
+          );
+        },
+        default: null,
+      });
+      let request = new Request("http://example.com/");
+      await handler(request);
+      expect(handleErrorSpy).not.toHaveBeenCalled();
+    });
+  });
+});
+
+// let request = new Request(
+//   "http://example.com/random?_data=routes/random&foo=bar",
+//   {
+//     method: "post",
+//     // headers: {
+//     //   "Content-Type": "application/json",
+//     // },
+//   }
+// );

packages/remix-server-runtime/__tests__/server-test.ts

@@ -1747,12 +1747,14 @@ describe("shared server runtime", () => {
       });
       let build = mockServerBuild({
         root: {
+          path: "/",
           default: {},
           loader: rootLoader,
           ErrorBoundary: {},
         },
         "routes/_index": {
           parentId: "root",
+          index: true,
           default: {},
           loader: indexLoader,
         },
@@ -1772,21 +1774,19 @@ describe("shared server runtime", () => {
       let result = await handler(request);
       expect(result.status).toBe(500);
       expect((await result.text()).includes(errorMessage)).toBe(true);
-      expect(rootLoader.mock.calls.length).toBe(0);
-      expect(indexLoader.mock.calls.length).toBe(0);
+      expect(rootLoader.mock.calls.length).toBe(1);
+      expect(indexLoader.mock.calls.length).toBe(1);
 
       let calls = build.entry.module.default.mock.calls;
       expect(calls.length).toBe(2);
-      expect(spy.console.mock.calls[0][0].data).toEqual(
-        'Error: No route matches URL "/"'
-      );
-      expect(spy.console.mock.calls[1][0].message).toEqual(
-        "thrown from handleDocumentRequest and expected to be logged in console only once"
-      );
-      expect(spy.console.mock.calls[2][0].message).toEqual(
-        "second error thrown from handleDocumentRequest"
-      );
-      expect(spy.console.mock.calls.length).toBe(3);
+      expect(spy.console.mock.calls).toEqual([
+        [
+          new Error(
+            "thrown from handleDocumentRequest and expected to be logged in console only once"
+          ),
+        ],
+        [new Error("second error thrown from handleDocumentRequest")],
+      ]);
     });
   });