Update replace logic for form submissions/redirects

.changeset/kind-gifts-reflect.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+Fix explicit `replace` on submissions and `PUSH` on submission to new paths

docs/components/form.md

@@ -179,11 +179,15 @@ Instructs the form to replace the current entry in the history stack, instead of
 <Form replace />
 ```
 
-The default behavior is conditional on the form `method`:
-
-- `get` defaults to `false`
-- every other method defaults to `true` if your `action` is successful
-- if your `action` redirects or throws, then it will still push by default
+The default behavior is conditional on the form behavior:
+
+- `method=get` forms default to `false`
+- submission methods depend on the `formAction` and `action` behavior:
+  - if your `action` throws, then it will default to `false`
+  - if your `action` redirects to the current location, it defaults to `true`
+  - if your `action` redirects elsewhere, it defaults to `false`
+  - if your `formAction` is the current location, it defaults to `true`
+  - otherwise it defaults to `false`
 
 We've found with `get` you often want the user to be able to click "back" to see the previous search results/filters, etc. But with the other methods the default is `true` to avoid the "are you sure you want to resubmit the form?" prompt. Note that even if `replace={false}` React Router _will not_ resubmit the form when the back button is clicked and the method is post, put, patch, or delete.
 

packages/react-router-dom/__tests__/data-browser-router-test.tsx

@@ -1311,7 +1311,7 @@ function testDomRouter(
       `);
     });
 
-    it('defaults useSubmit({ method: "post" }) to be a REPLACE navigation', async () => {
+    it('defaults useSubmit({ method: "post" }) to a new location to be a PUSH navigation', async () => {
       let { container } = render(
         <TestDataRouter window={getWindow("/")} hydrationData={{}}>
           <Route element={<Layout />}>
@@ -1387,6 +1387,100 @@ function testDomRouter(
         </div>"
       `);
 
+      fireEvent.click(screen.getByText("Go back"));
+      await waitFor(() => screen.getByText("Page 1"));
+      expect(getHtml(container.querySelector(".output")))
+        .toMatchInlineSnapshot(`
+        "<div
+          class=\\"output\\"
+        >
+          <h1>
+            Page 1
+          </h1>
+        </div>"
+      `);
+    });
+
+    it('defaults useSubmit({ method: "post" }) to the same location to be a REPLACE navigation', async () => {
+      let { container } = render(
+        <TestDataRouter window={getWindow("/")} hydrationData={{}}>
+          <Route element={<Layout />}>
+            <Route index loader={() => "index"} element={<h1>index</h1>} />
+            <Route
+              path="1"
+              action={() => "action"}
+              loader={() => "1"}
+              element={<h1>Page 1</h1>}
+            />
+          </Route>
+        </TestDataRouter>
+      );
+
+      function Layout() {
+        let navigate = useNavigate();
+        let submit = useSubmit();
+        let actionData = useActionData();
+        let formData = new FormData();
+        formData.append("test", "value");
+        return (
+          <>
+            <Link to="1">Go to 1</Link>
+            <button
+              onClick={() => {
+                submit(formData, { action: "1", method: "post" });
+              }}
+            >
+              Submit
+            </button>
+            <button onClick={() => navigate(-1)}>Go back</button>
+            <div className="output">
+              {actionData ? <p>{actionData}</p> : null}
+              <Outlet />
+            </div>
+          </>
+        );
+      }
+
+      expect(getHtml(container.querySelector(".output")))
+        .toMatchInlineSnapshot(`
+        "<div
+          class=\\"output\\"
+        >
+          <h1>
+            index
+          </h1>
+        </div>"
+      `);
+
+      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>"
+      `);
+
+      fireEvent.click(screen.getByText("Submit"));
+      await waitFor(() => screen.getByText("action"));
+      expect(getHtml(container.querySelector(".output")))
+        .toMatchInlineSnapshot(`
+        "<div
+          class=\\"output\\"
+        >
+          <p>
+            action
+          </p>
+          <h1>
+            Page 1
+          </h1>
+        </div>"
+      `);
+
       fireEvent.click(screen.getByText("Go back"));
       await waitFor(() => screen.getByText("index"));
       expect(getHtml(container.querySelector(".output")))

packages/router/__tests__/router-test.ts

@@ -2519,6 +2519,35 @@ describe("a router", () => {
       expect(t.router.state.location.pathname).toEqual("/foo");
     });
 
+    it("navigates correctly using POP navigations across actions to new locations", 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 /baz (should not replace)
+      let C = await t.navigate("/baz", {
+        formMethod: "post",
+        formData: createFormData({ key: "value" }),
+      });
+      await C.actions.baz.resolve("BAZ ACTION");
+      await C.loaders.root.resolve("ROOT");
+      await C.loaders.baz.resolve("BAZ");
+      expect(t.router.state.location.pathname).toEqual("/baz");
+
+      // POP to /bar
+      let D = await t.navigate(-1);
+      await D.loaders.bar.resolve("BAR");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+    });
+
     it("navigates correctly using POP navigations across action errors", async () => {
       let t = initializeTmTest();
 
@@ -2635,6 +2664,42 @@ describe("a router", () => {
       expect(t.router.state.location.key).not.toBe(postBarKey);
     });
 
+    it("navigates correctly using POP navigations across action redirects to the same location", async () => {
+      let t = initializeTmTest();
+
+      // Navigate to /foo
+      let A = await t.navigate("/foo");
+      let fooKey = t.router.state.navigation.location?.key;
+      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.historyAction).toEqual("PUSH");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+
+      // Post to /bar, redirect to /bar
+      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("/bar");
+      await D.loaders.root.resolve("ROOT");
+      await D.loaders.bar.resolve("BAR");
+      expect(t.router.state.historyAction).toEqual("REPLACE");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+
+      // POP to /foo
+      let E = await t.navigate(-1);
+      await E.loaders.foo.resolve("FOO");
+      expect(t.router.state.historyAction).toEqual("POP");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+      expect(t.router.state.location.key).toBe(fooKey);
+      expect(t.router.state.location.key).not.toBe(postBarKey);
+    });
+
     it("navigates correctly using POP navigations across <Form replace> redirects", async () => {
       let t = initializeTmTest();
 
@@ -2667,6 +2732,67 @@ describe("a router", () => {
       expect(t.router.state.historyAction).toEqual("POP");
       expect(t.router.state.location.pathname).toEqual("/foo");
     });
+
+    it("should respect explicit replace:false on non-redirected actions to new locations", async () => {
+      // start at / (history stack: [/])
+      let t = initializeTmTest();
+
+      // Link to /foo (history stack: [/, /foo])
+      let A = await t.navigate("/foo");
+      await A.loaders.root.resolve("ROOT");
+      await A.loaders.foo.resolve("FOO");
+      expect(t.router.state.historyAction).toEqual("PUSH");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+
+      // POST /bar (history stack: [/, /foo, /bar])
+      let B = await t.navigate("/bar", {
+        formMethod: "post",
+        formData: createFormData({ gosh: "dang" }),
+        replace: false,
+      });
+      await B.actions.bar.resolve("BAR");
+      await B.loaders.root.resolve("ROOT");
+      await B.loaders.bar.resolve("BAR");
+      expect(t.router.state.historyAction).toEqual("PUSH");
+      expect(t.router.state.location.pathname).toEqual("/bar");
+
+      // POP /foo (history stack: [GET /, GET /foo])
+      let C = await t.navigate(-1);
+      await C.loaders.foo.resolve("FOO");
+      expect(t.router.state.historyAction).toEqual("POP");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+    });
+
+    it("should respect explicit replace:false on non-redirected actions to the same location", async () => {
+      // start at / (history stack: [/])
+      let t = initializeTmTest();
+
+      // Link to /foo (history stack: [/, /foo])
+      let A = await t.navigate("/foo");
+      await A.loaders.root.resolve("ROOT");
+      await A.loaders.foo.resolve("FOO");
+      expect(t.router.state.historyAction).toEqual("PUSH");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+
+      // POST /foo (history stack: [/, /foo, /foo])
+      let B = await t.navigate("/foo", {
+        formMethod: "post",
+        formData: createFormData({ gosh: "dang" }),
+        replace: false,
+      });
+      await B.actions.foo.resolve("FOO2 ACTION");
+      await B.loaders.root.resolve("ROOT2");
+      await B.loaders.foo.resolve("FOO2");
+      expect(t.router.state.historyAction).toEqual("PUSH");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+
+      // POP /foo (history stack: [/, /foo])
+      let C = await t.navigate(-1);
+      await C.loaders.root.resolve("ROOT3");
+      await C.loaders.foo.resolve("FOO3");
+      expect(t.router.state.historyAction).toEqual("POP");
+      expect(t.router.state.location.pathname).toEqual("/foo");
+    });
   });
 
   describe("submission navigations", () => {
@@ -6384,7 +6510,7 @@ describe("a router", () => {
       await N.loaders.root.resolve("ROOT_DATA*");
       await N.loaders.tasks.resolve("TASKS_DATA");
       expect(t.router.state).toMatchObject({
-        historyAction: "REPLACE",
+        historyAction: "PUSH",
         location: { pathname: "/tasks" },
         navigation: IDLE_NAVIGATION,
         revalidation: "idle",
@@ -6396,7 +6522,7 @@ describe("a router", () => {
           tasks: "TASKS_ACTION",
         },
       });
-      expect(t.history.replace).toHaveBeenCalledWith(
+      expect(t.history.push).toHaveBeenCalledWith(
         t.router.state.location,
         t.router.state.location.state
       );
@@ -6596,7 +6722,7 @@ describe("a router", () => {
       await R.loaders.root.resolve("ROOT_DATA*");
       await R.loaders.tasks.resolve("TASKS_DATA*");
       expect(t.router.state).toMatchObject({
-        historyAction: "REPLACE",
+        historyAction: "PUSH",
         location: { pathname: "/tasks" },
         navigation: IDLE_NAVIGATION,
         revalidation: "idle",
@@ -6605,7 +6731,7 @@ describe("a router", () => {
           tasks: "TASKS_DATA*",
         },
       });
-      expect(t.history.replace).toHaveBeenCalledWith(
+      expect(t.history.push).toHaveBeenCalledWith(
         t.router.state.location,
         t.router.state.location.state
       );
@@ -6683,7 +6809,7 @@ describe("a router", () => {
       await R.loaders.root.resolve("ROOT_DATA*");
       await R.loaders.tasks.resolve("TASKS_DATA*");
       expect(t.router.state).toMatchObject({
-        historyAction: "REPLACE",
+        historyAction: "PUSH",
         location: { pathname: "/tasks" },
         navigation: IDLE_NAVIGATION,
         revalidation: "idle",
@@ -6695,7 +6821,7 @@ describe("a router", () => {
           tasks: "TASKS_ACTION",
         },
       });
-      expect(t.history.replace).toHaveBeenCalledWith(
+      expect(t.history.push).toHaveBeenCalledWith(
         t.router.state.location,
         t.router.state.location.state
       );

packages/router/router.ts

@@ -821,11 +821,27 @@ export function createRouter(init: RouterInit): Router {
       ...init.history.encodeLocation(location),
     };
 
-    let historyAction =
-      (opts && opts.replace) === true ||
-      (submission != null && isMutationMethod(submission.formMethod))
-        ? HistoryAction.Replace
-        : HistoryAction.Push;
+    let userReplace = opts && opts.replace != null ? opts.replace : undefined;
+
+    let historyAction = HistoryAction.Push;
+
+    if (userReplace === true) {
+      historyAction = HistoryAction.Replace;
+    } else if (userReplace === false) {
+      // no-op
+    } else if (submission != null && isMutationMethod(submission.formMethod)) {
+      // By default on submissions to the current location we REPLACE so that
+      // users don't have to double-click the back button to get to the prior
+      // location.  If the user redirects from the action/loader this will be
+      // ignored and the redirect will be a PUSH
+      if (
+        submission.formAction ===
+        state.location.pathname + state.location.search
+      ) {
+        historyAction = HistoryAction.Replace;
+      }
+    }
+
     let preventScrollReset =
       opts && "preventScrollReset" in opts
         ? opts.preventScrollReset === true
@@ -1054,11 +1070,17 @@ export function createRouter(init: RouterInit): Router {
     }
 
     if (isRedirectResult(result)) {
-      await startRedirectNavigation(
-        state,
-        result,
-        opts && opts.replace === true
-      );
+      let replace: boolean;
+      if (opts && opts.replace != null) {
+        replace = opts.replace;
+      } else {
+        // If the user didn't explicity indicate replace behavior, replace if
+        // we redirected to the exact same location we're currently at to avoid
+        // double back-buttons
+        replace =
+          result.location === state.location.pathname + state.location.search;
+      }
+      await startRedirectNavigation(state, result, replace);
       return { shortCircuited: true };
     }