Repository: remix-run/remix
Pull request #3436 (2022-06-10 13:04:48)

fix(remix-server-runtime): use correct error/catch boundary on SSR action errors
Closes: #599

- [x] Tests

Testing Strategy: I tested using the repo in #599 but the repro setup is basically:
* Parent route with loader data
* Child route that exports `ErrorBoundary` or `CatchBoundary`
* Child contains `form` or `<Form reloadDocument={true}>` submitting to child action that throws
  * This error only happens in server-side form submissions 
* Proper boundary should be used and the parent loaderData should not be wiped away on submission


Patch:
diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts
index c785fab83..ac933ef2d 100644
--- a/packages/remix-server-runtime/server.ts
+++ b/packages/remix-server-runtime/server.ts
@@ -273,24 +273,21 @@ async function handleDocumentRequest({
   let routeModules = createEntryRouteModules(build.routes);
 
   let matchesToLoad = matches || [];
+
+  // get rid of the action, we don't want to call it's loader either
+  // because we'll be rendering the error/catch boundary, if you can get
+  // access to the loader data in the error/catch boundary then how the heck
+  // is it supposed to deal with thrown responses and/or errors in the loader?
   if (appState.catch) {
     matchesToLoad = getMatchesUpToDeepestBoundary(
-      // get rid of the action, we don't want to call it's loader either
-      // because we'll be rendering the catch boundary, if you can get access
-      // to the loader data in the catch boundary then how the heck is it
-      // supposed to deal with thrown responses?
-      matchesToLoad.slice(0, -1),
+      matchesToLoad,
       "CatchBoundary"
-    );
+    ).slice(0, -1);
   } else if (appState.error) {
     matchesToLoad = getMatchesUpToDeepestBoundary(
-      // get rid of the action, we don't want to call it's loader either
-      // because we'll be rendering the error boundary, if you can get access
-      // to the loader data in the error boundary then how the heck is it
-      // supposed to deal with errors in the loader, too?
-      matchesToLoad.slice(0, -1),
+      matchesToLoad,
       "ErrorBoundary"
-    );
+    ).slice(0, -1);
   }
 
   let loaderRequest = new Request(request.url, {


Test patch:
diff --git a/integration/catch-boundary-test.ts b/integration/catch-boundary-test.ts
index a4522a428..3323deff6 100644
--- a/integration/catch-boundary-test.ts
+++ b/integration/catch-boundary-test.ts
@@ -182,6 +182,53 @@ test.describe("CatchBoundary", () => {
             return <div/>
           }
         `,
+
+        "app/routes/action.jsx": js`
+          import { Outlet, useLoaderData } from "@remix-run/react";
+
+          export function loader() {
+            return "PARENT";
+          }
+
+          export default function () {
+            return (
+              <div>
+                <p id="parent-data">{useLoaderData()}</p>
+                <Outlet />
+              </div>
+            )
+          }
+        `,
+
+        "app/routes/action/child-catch.jsx": js`
+          import { Form, useCatch, useLoaderData } from "@remix-run/react";
+
+          export function loader() {
+            return "CHILD";
+          }
+
+          export function action() {
+            throw new Response("Caught!", { status: 400 });
+          }
+
+          export default function () {
+            return (
+              <>
+                <p id="child-data">{useLoaderData()}</p>
+                <Form method="post" reloadDocument={true}>
+                  <button type="submit" name="key" value="value">
+                    Submit
+                  </button>
+                </Form>
+              </>
+            )
+          }
+
+          export function CatchBoundary() {
+            let caught = useCatch()
+            return <p id="child-catch">{caught.status} {caught.data}</p>;
+          }
+        `,
       },
     });
 
@@ -338,4 +385,19 @@ test.describe("CatchBoundary", () => {
     await app.clickSubmitButton(NO_BOUNDARY_NO_LOADER_OR_ACTION);
     expect(await app.getHtml()).toMatch(ROOT_BOUNDARY_TEXT);
   });
+
+  test("uses correct catch boundary on server action errors", async ({
+    page,
+  }) => {
+    let app = new PlaywrightFixture(appFixture, page);
+    await app.goto(`/action/child-catch`);
+    expect(await app.getHtml("#parent-data")).toMatch("PARENT");
+    expect(await app.getHtml("#child-data")).toMatch("CHILD");
+    await page.click("button[type=submit]");
+    await page.waitForSelector("#child-catch");
+    // Preserves parent loader data
+    expect(await app.getHtml("#parent-data")).toMatch("PARENT");
+    expect(await app.getHtml("#child-catch")).toMatch("400");
+    expect(await app.getHtml("#child-catch")).toMatch("Caught!");
+  });
 });
diff --git a/integration/error-boundary-test.ts b/integration/error-boundary-test.ts
index 011e60ba0..92c94327e 100644
--- a/integration/error-boundary-test.ts
+++ b/integration/error-boundary-test.ts
@@ -171,6 +171,52 @@ test.describe("ErrorBoundary", () => {
             return <div>${OWN_BOUNDARY_TEXT}</div>
           }
         `,
+
+        "app/routes/action.jsx": js`
+          import { Outlet, useLoaderData } from "@remix-run/react";
+
+          export function loader() {
+            return "PARENT";
+          }
+
+          export default function () {
+            return (
+              <div>
+                <p id="parent-data">{useLoaderData()}</p>
+                <Outlet />
+              </div>
+            )
+          }
+        `,
+
+        "app/routes/action/child-error.jsx": js`
+          import { Form, useLoaderData } from "@remix-run/react";
+
+          export function loader() {
+            return "CHILD";
+          }
+
+          export function action() {
+            throw new Error("Broken!");
+          }
+
+          export default function () {
+            return (
+              <>
+                <p id="child-data">{useLoaderData()}</p>
+                <Form method="post" reloadDocument={true}>
+                  <button type="submit" name="key" value="value">
+                    Submit
+                  </button>
+                </Form>
+              </>
+            )
+          }
+
+          export function ErrorBoundary({ error }) {
+            return <p id="child-error">{error.message}</p>;
+          }
+        `,
       },
     });
 
@@ -293,6 +339,20 @@ test.describe("ErrorBoundary", () => {
     expect(await app.getHtml("main")).toMatch(OWN_BOUNDARY_TEXT);
   });
 
+  test("uses correct error boundary on server action errors in nested routes", async ({
+    page,
+  }) => {
+    let app = new PlaywrightFixture(appFixture, page);
+    await app.goto(`/action/child-error`);
+    expect(await app.getHtml("#parent-data")).toMatch("PARENT");
+    expect(await app.getHtml("#child-data")).toMatch("CHILD");
+    await page.click("button[type=submit]");
+    await page.waitForSelector("#child-error");
+    // Preserves parent loader data
+    expect(await app.getHtml("#parent-data")).toMatch("PARENT");
+    expect(await app.getHtml("#child-error")).toMatch("Broken!");
+  });
+
   test.describe("if no error boundary exists in the app", () => {
     let NO_ROOT_BOUNDARY_LOADER = "/loader-bad";
     let NO_ROOT_BOUNDARY_ACTION = "/action-bad";
diff --git a/packages/remix-server-runtime/__tests__/server-test.ts b/packages/remix-server-runtime/__tests__/server-test.ts
index 93a3087e6..5c691c513 100644
--- a/packages/remix-server-runtime/__tests__/server-test.ts
+++ b/packages/remix-server-runtime/__tests__/server-test.ts
@@ -880,7 +880,8 @@ describe("shared server runtime", () => {
       let result = await handler(request);
       expect(result.status).toBe(400);
       expect(testAction.mock.calls.length).toBe(1);
-      expect(rootLoader.mock.calls.length).toBe(1);
+      // Should not call root loader since it is the boundary route
+      expect(rootLoader.mock.calls.length).toBe(0);
       expect(testLoader.mock.calls.length).toBe(0);
       expect(build.entry.module.default.mock.calls.length).toBe(1);
 
@@ -890,9 +891,7 @@ describe("shared server runtime", () => {
       expect(entryContext.appState.catch).toBeTruthy();
       expect(entryContext.appState.catch!.status).toBe(400);
       expect(entryContext.appState.catchBoundaryRouteId).toBe("root");
-      expect(entryContext.routeData).toEqual({
-        root: "root",
-      });
+      expect(entryContext.routeData).toEqual({});
     });
 
     test("thrown action responses bubble up for index routes", async () => {
@@ -926,7 +925,8 @@ describe("shared server runtime", () => {
       let result = await handler(request);
       expect(result.status).toBe(400);
       expect(indexAction.mock.calls.length).toBe(1);
-      expect(rootLoader.mock.calls.length).toBe(1);
+      // Should not call root loader since it is the boundary route
+      expect(rootLoader.mock.calls.length).toBe(0);
       expect(indexLoader.mock.calls.length).toBe(0);
       expect(build.entry.module.default.mock.calls.length).toBe(1);
 
@@ -936,9 +936,7 @@ describe("shared server runtime", () => {
       expect(entryContext.appState.catch).toBeTruthy();
       expect(entryContext.appState.catch!.status).toBe(400);
       expect(entryContext.appState.catchBoundaryRouteId).toBe("root");
-      expect(entryContext.routeData).toEqual({
-        root: "root",
-      });
+      expect(entryContext.routeData).toEqual({});
     });
 
     test("thrown action responses catch deep", async () => {
@@ -1263,7 +1261,8 @@ describe("shared server runtime", () => {
       let result = await handler(request);
       expect(result.status).toBe(500);
       expect(testAction.mock.calls.length).toBe(1);
-      expect(rootLoader.mock.calls.length).toBe(1);
+      // Should not call root loader since it is the boundary route
+      expect(rootLoader.mock.calls.length).toBe(0);
       expect(testLoader.mock.calls.length).toBe(0);
       expect(build.entry.module.default.mock.calls.length).toBe(1);
 
@@ -1273,9 +1272,7 @@ describe("shared server runtime", () => {
       expect(entryContext.appState.error).toBeTruthy();
       expect(entryContext.appState.error.message).toBe("test");
       expect(entryContext.appState.loaderBoundaryRouteId).toBe("root");
-      expect(entryContext.routeData).toEqual({
-        root: "root",
-      });
+      expect(entryContext.routeData).toEqual({});
     });
 
     test("action errors bubble up for index routes", async () => {
@@ -1309,7 +1306,8 @@ describe("shared server runtime", () => {
       let result = await handler(request);
       expect(result.status).toBe(500);
       expect(indexAction.mock.calls.length).toBe(1);
-      expect(rootLoader.mock.calls.length).toBe(1);
+      // Should not call root loader since it is the boundary route
+      expect(rootLoader.mock.calls.length).toBe(0);
       expect(indexLoader.mock.calls.length).toBe(0);
       expect(build.entry.module.default.mock.calls.length).toBe(1);
 
@@ -1319,9 +1317,7 @@ describe("shared server runtime", () => {
       expect(entryContext.appState.error).toBeTruthy();
       expect(entryContext.appState.error.message).toBe("index");
       expect(entryContext.appState.loaderBoundaryRouteId).toBe("root");
-      expect(entryContext.routeData).toEqual({
-        root: "root",
-      });
+      expect(entryContext.routeData).toEqual({});
     });
 
     test("action errors catch deep", async () => {