fix: do not REPLACE if action redirected (#8979)

brophdawg11 · web-flow · commit 7a057e19705e · 2022-06-22T07:15:00.000-06:00
* fix: do not REPLACE if action redirected

* add changeset

* Enhance test to assert correct location key
diff --git a/.changeset/odd-yaks-kneel.md b/.changeset/odd-yaks-kneel.md
@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+fix: don't default to a `REPLACE` navigation on form submissions if the action redirected. The redirect takes care of avoiding the back-button-resubmit scenario, so by using a `PUSH` we allow the back button to go back to the pre-submission form page (#8979)
diff --git a/packages/react-router-dom/__tests__/DataBrowserRouter-test.tsx b/packages/react-router-dom/__tests__/DataBrowserRouter-test.tsx
@@ -772,14 +772,13 @@ function testDomRouter(
       let { container } = render(
         <TestDataRouter window={getWindow("/")} hydrationData={{}}>
           <Route element={<Layout />}>
-            <Route index loader={() => "index"} element={<h1>index</h1>} />
-            <Route path="1" loader={() => "1"} element={<h1>Page 1</h1>} />
+            <Route index loader={() => "index"} element={<h1>Index Page</h1>} />
             <Route
-              path="2"
-              action={() => "action"}
-              loader={() => "2"}
-              element={<h1>Page 2</h1>}
+              path="form"
+              action={() => "action data"}
+              element={<FormPage />}
             />
+            <Route path="result" element={<h1>Result Page</h1>} />
           </Route>
         </TestDataRouter>
       );
@@ -788,11 +787,7 @@ function testDomRouter(
         let navigate = useNavigate();
         return (
           <>
-            <Link to="1">Go to 1</Link>
-            <Form action="2" method="post">
-              <input name="test" defaultValue="value" />
-              <button type="submit">Submit Form</button>
-            </Form>
+            <Link to="form">Go to Form</Link>
             <button onClick={() => navigate(-1)}>Go back</button>
             <div className="output">
               <Outlet />
@@ -801,55 +796,100 @@ function testDomRouter(
         );
       }
 
-      expect(getHtml(container.querySelector(".output")))
-        .toMatchInlineSnapshot(`
-        "<div
-          class=\\"output\\"
-        >
-          <h1>
-            index
-          </h1>
-        </div>"
-      `);
+      function FormPage() {
+        let data = useActionData();
+        return (
+          <Form method="post">
+            <p>Form Page</p>
+            <p>{data}</p>
+            <input name="test" defaultValue="value" />
+            <button type="submit">Submit</button>
+          </Form>
+        );
+      }
 
-      fireEvent.click(screen.getByText("Go to 1"));
-      await waitFor(() => screen.getByText("Page 1"));
-      expect(getHtml(container.querySelector(".output")))
-        .toMatchInlineSnapshot(`
-        "<div
-          class=\\"output\\"
-        >
-          <h1>
-            Page 1
-          </h1>
-        </div>"
-      `);
+      let html = () => getHtml(container.querySelector(".output"));
 
-      fireEvent.click(screen.getByText("Submit Form"));
-      await waitFor(() => screen.getByText("Page 2"));
-      expect(getHtml(container.querySelector(".output")))
-        .toMatchInlineSnapshot(`
-        "<div
-          class=\\"output\\"
-        >
-          <h1>
-            Page 2
-          </h1>
-        </div>"
-      `);
+      // Start on index page
+      expect(html()).toMatch("Index Page");
+
+      // Navigate to form page
+      fireEvent.click(screen.getByText("Go to Form"));
+      await waitFor(() => screen.getByText("Form Page"));
+      expect(html()).not.toMatch("action result");
 
+      // Submit without redirect does a replace
+      fireEvent.click(screen.getByText("Submit"));
+      await waitFor(() => screen.getByText("action data"));
+      expect(html()).toMatch("Form Page");
+      expect(html()).toMatch("action data");
+
+      // Back navigate to index page
       fireEvent.click(screen.getByText("Go back"));
-      await waitFor(() => screen.getByText("index"));
-      expect(getHtml(container.querySelector(".output")))
-        .toMatchInlineSnapshot(`
-        "<div
-          class=\\"output\\"
-        >
-          <h1>
-            index
-          </h1>
-        </div>"
-      `);
+      await waitFor(() => screen.getByText("Index Page"));
+    });
+
+    it('Uses a PUSH navigation on <Form method="post"> if it redirects', async () => {
+      let { container } = render(
+        <TestDataRouter window={getWindow("/")} hydrationData={{}}>
+          <Route element={<Layout />}>
+            <Route index loader={() => "index"} element={<h1>Index Page</h1>} />
+            <Route
+              path="form"
+              action={() =>
+                new Response(null, {
+                  status: 302,
+                  headers: { Location: "/result" },
+                })
+              }
+              element={<FormPage />}
+            />
+            <Route path="result" element={<h1>Result Page</h1>} />
+          </Route>
+        </TestDataRouter>
+      );
+
+      function Layout() {
+        let navigate = useNavigate();
+        return (
+          <>
+            <Link to="form">Go to Form</Link>
+            <button onClick={() => navigate(-1)}>Go back</button>
+            <div className="output">
+              <Outlet />
+            </div>
+          </>
+        );
+      }
+
+      function FormPage() {
+        let data = useActionData();
+        return (
+          <Form method="post">
+            <p>Form Page</p>
+            <p>{data}</p>
+            <input name="test" defaultValue="value" />
+            <button type="submit">Submit</button>
+          </Form>
+        );
+      }
+
+      let html = () => getHtml(container.querySelector(".output"));
+
+      // Start on index page
+      expect(html()).toMatch("Index Page");
+
+      // Navigate to form page
+      fireEvent.click(screen.getByText("Go to Form"));
+      await waitFor(() => screen.getByText("Form Page"));
+
+      // Submit with redirect
+      fireEvent.click(screen.getByText("Submit"));
+      await waitFor(() => screen.getByText("Result Page"));
+
+      // Back navigate to form page
+      fireEvent.click(screen.getByText("Go back"));
+      await waitFor(() => screen.getByText("Form Page"));
     });
 
     it('defaults useSubmit({ method: "get" }) to be a PUSH navigation', async () => {
diff --git a/packages/react-router-dom/index.tsx b/packages/react-router-dom/index.tsx
@@ -775,10 +775,7 @@ function useSubmitImpl(fetcherKey?: string, routeId?: string): SubmitFunction {
 
       let href = url.pathname + url.search;
       let opts = {
-        // If replace is not specified, we'll default to false for GET and
-        // true otherwise
-        replace:
-          options.replace != null ? options.replace === true : method !== "get",
+        replace: options.replace,
         formData,
         formMethod: method as FormMethod,
         formEncType: encType as FormEncType,
diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts
@@ -1955,7 +1955,7 @@ describe("a router", () => {
     });
   });
 
-  describe("POP navigations after action redirect", () => {
+  describe("POP navigations", () => {
     it("does a normal load when backing into an action redirect", async () => {
       let t = initializeTmTest();
       let A = await t.navigate("/foo", {
@@ -1979,14 +1979,131 @@ describe("a router", () => {
         baz: "C LOADER",
       });
 
-      let D = await t.navigate(-2);
+      let D = await t.navigate(-1);
       await D.loaders.bar.resolve("D LOADER");
       expect(D.loaders.root.stub.mock.calls.length).toBe(0);
       expect(t.router.state.loaderData).toMatchObject({
         root: "ROOT DATA",
         bar: "D LOADER",
       });
     });
+
+    it("navigates correctly using POP navigations", async () => {
+      let t = initializeTmTest();
+
+      let A = await t.navigate("/foo");
+      await A.loaders.foo.resolve("FOO");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+
+      let B = await t.navigate("/bar");
+      await B.loaders.bar.resolve("BAR");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+
+      let C = await t.navigate(-1);
+      await C.loaders.foo.resolve("FOO*");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+
+      let D = await t.navigate("/baz", { replace: true });
+      await D.loaders.baz.resolve("BAZ");
+      expect(t.router.state.location.pathname).toEqual("/baz");
+
+      // POP to /
+      let E = await t.navigate(-1);
+      await E.loaders.index.resolve("INDEX*");
+      expect(t.router.state.location.pathname).toEqual("/");
+    });
+
+    it("navigates correctly using POP navigations across actions", async () => {
+      let t = initializeTmTest();
+
+      // Navigate to /foo
+      let A = await t.navigate("/foo");
+      await A.loaders.foo.resolve("FOO");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+
+      // Navigate to /bar
+      let B = await t.navigate("/bar");
+      await B.loaders.bar.resolve("BAR");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+
+      // Post to /bar (should replace)
+      let C = await t.navigate("/bar", {
+        formMethod: "post",
+        formData: createFormData({ key: "value" }),
+      });
+      await C.actions.bar.resolve("BAR ACTION");
+      await C.loaders.root.resolve("ROOT");
+      await C.loaders.bar.resolve("BAR");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+
+      // POP to /foo
+      let D = await t.navigate(-1);
+      await D.loaders.foo.resolve("FOO");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+    });
+
+    it("navigates correctly using POP navigations across action redirects", async () => {
+      let t = initializeTmTest();
+
+      // Navigate to /foo
+      let A = await t.navigate("/foo");
+      await A.loaders.foo.resolve("FOO");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+
+      // Navigate to /bar
+      let B = await t.navigate("/bar");
+      let getBarKey = t.router.state.navigation.location?.key;
+      await B.loaders.bar.resolve("BAR");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+
+      // Post to /bar, redirect to /baz
+      let C = await t.navigate("/bar", {
+        formMethod: "post",
+        formData: createFormData({ key: "value" }),
+      });
+      let postBarKey = t.router.state.navigation.location?.key;
+      let D = await C.actions.bar.redirect("/baz");
+      await D.loaders.root.resolve("ROOT");
+      await D.loaders.baz.resolve("BAZ");
+      expect(t.router.state.location.pathname).toEqual("/baz");
+
+      // POP to /bar
+      let E = await t.navigate(-1);
+      await E.loaders.bar.resolve("BAR");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+      expect(t.router.state.location.key).toBe(getBarKey);
+      expect(t.router.state.location.key).not.toBe(postBarKey);
+    });
+
+    it("navigates correctly using POP navigations across <Form replace> redirects", async () => {
+      let t = initializeTmTest();
+
+      // Navigate to /foo
+      let A = await t.navigate("/foo");
+      await A.loaders.foo.resolve("FOO");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+
+      // Navigate to /bar
+      let B = await t.navigate("/bar");
+      await B.loaders.bar.resolve("BAR");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+
+      // Post to /bar, redirect to /baz
+      let C = await t.navigate("/bar", {
+        formMethod: "post",
+        formData: createFormData({ key: "value" }),
+        replace: true,
+      });
+      let D = await C.actions.bar.redirect("/baz");
+      await D.loaders.root.resolve("ROOT");
+      await D.loaders.baz.resolve("BAZ");
+      expect(t.router.state.location.pathname).toEqual("/baz");
+
+      // POP to /foo
+      let E = await t.navigate(-1);
+      await E.loaders.foo.resolve("FOO");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+    });
   });
 
   describe("submission navigations", () => {
@@ -4602,7 +4719,7 @@ describe("a router", () => {
       await N.loaders.root.resolve("ROOT_DATA*");
       await N.loaders.tasks.resolve("TASKS_DATA");
       expect(t.router.state).toMatchObject({
-        historyAction: "PUSH",
+        historyAction: "REPLACE",
         location: { pathname: "/tasks" },
         navigation: IDLE_NAVIGATION,
         revalidation: "idle",
@@ -4614,7 +4731,7 @@ describe("a router", () => {
           tasks: "TASKS_ACTION",
         },
       });
-      expect(t.history.push).toHaveBeenCalledWith(
+      expect(t.history.replace).toHaveBeenCalledWith(
         t.router.state.location,
         t.router.state.location.state
       );
@@ -4814,7 +4931,7 @@ describe("a router", () => {
       await R.loaders.root.resolve("ROOT_DATA*");
       await R.loaders.tasks.resolve("TASKS_DATA*");
       expect(t.router.state).toMatchObject({
-        historyAction: "PUSH",
+        historyAction: "REPLACE",
         location: { pathname: "/tasks" },
         navigation: IDLE_NAVIGATION,
         revalidation: "idle",
@@ -4823,7 +4940,7 @@ describe("a router", () => {
           tasks: "TASKS_DATA*",
         },
       });
-      expect(t.history.push).toHaveBeenCalledWith(
+      expect(t.history.replace).toHaveBeenCalledWith(
         t.router.state.location,
         t.router.state.location.state
       );
@@ -4901,7 +5018,7 @@ describe("a router", () => {
       await R.loaders.root.resolve("ROOT_DATA*");
       await R.loaders.tasks.resolve("TASKS_DATA*");
       expect(t.router.state).toMatchObject({
-        historyAction: "PUSH",
+        historyAction: "REPLACE",
         location: { pathname: "/tasks" },
         navigation: IDLE_NAVIGATION,
         revalidation: "idle",
@@ -4913,7 +5030,7 @@ describe("a router", () => {
           tasks: "TASKS_ACTION",
         },
       });
-      expect(t.history.push).toHaveBeenCalledWith(
+      expect(t.history.replace).toHaveBeenCalledWith(
         t.router.state.location,
         t.router.state.location.state
       );
diff --git a/packages/router/router.ts b/packages/router/router.ts

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"@remix-run/router": patch
 +---
++
 +fix: don't default to a `REPLACE` navigation on form submissions if the action redirected. The redirect takes care of avoiding the back-button-resubmit scenario, so by using a `PUSH` we allow the back button to go back to the pre-submission form page (#8979)