From 60ea5502aba66f6f95be911388bf6d7489580f9d Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 17:24:06 -0500 Subject: [PATCH 1/5] Update replace logic for form submissions/redirects --- .../__tests__/data-browser-router-test.tsx | 96 +++++++++++- packages/router/__tests__/router-test.ts | 144 ++++++++++++++++-- packages/router/router.ts | 42 +++-- 3 files changed, 262 insertions(+), 20 deletions(-) diff --git a/packages/react-router-dom/__tests__/data-browser-router-test.tsx b/packages/react-router-dom/__tests__/data-browser-router-test.tsx index 0f7d0334ce..b1c8ebace3 100644 --- a/packages/react-router-dom/__tests__/data-browser-router-test.tsx +++ b/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( }> @@ -1387,6 +1387,100 @@ function testDomRouter( " `); + fireEvent.click(screen.getByText("Go back")); + await waitFor(() => screen.getByText("Page 1")); + expect(getHtml(container.querySelector(".output"))) + .toMatchInlineSnapshot(` + "
+

+ Page 1 +

+
" + `); + }); + + it('defaults useSubmit({ method: "post" }) to the same location to be a REPLACE navigation', async () => { + let { container } = render( + + }> + "index"} element={

index

} /> + "action"} + loader={() => "1"} + element={

Page 1

} + /> + + + ); + + function Layout() { + let navigate = useNavigate(); + let submit = useSubmit(); + let actionData = useActionData(); + let formData = new FormData(); + formData.append("test", "value"); + return ( + <> + Go to 1 + + +
+ {actionData ?

{actionData}

: null} + +
+ + ); + } + + expect(getHtml(container.querySelector(".output"))) + .toMatchInlineSnapshot(` + "
+

+ index +

+
" + `); + + fireEvent.click(screen.getByText("Go to 1")); + await waitFor(() => screen.getByText("Page 1")); + expect(getHtml(container.querySelector(".output"))) + .toMatchInlineSnapshot(` + "
+

+ Page 1 +

+
" + `); + + fireEvent.click(screen.getByText("Submit")); + await waitFor(() => screen.getByText("action")); + expect(getHtml(container.querySelector(".output"))) + .toMatchInlineSnapshot(` + "
+

+ action +

+

+ Page 1 +

+
" + `); + fireEvent.click(screen.getByText("Go back")); await waitFor(() => screen.getByText("index")); expect(getHtml(container.querySelector(".output"))) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 313b503df6..d2e3acfaec 100644 --- a/packages/router/__tests__/router-test.ts +++ b/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
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 ); @@ -6998,7 +7124,7 @@ describe("a router", () => { let key = "key"; router.fetch(key, "root", "/"); - expect(router.state.fetchers.get(key)).toEqual({ + expect(router.state.fetchers.get(key)).toMatchObject({ state: "loading", formMethod: undefined, formEncType: undefined, @@ -7008,7 +7134,7 @@ describe("a router", () => { }); await dfd.resolve("DATA"); - expect(router.state.fetchers.get(key)).toEqual({ + expect(router.state.fetchers.get(key)).toMatchObject({ state: "idle", formMethod: undefined, formEncType: undefined, @@ -7500,7 +7626,7 @@ describe("a router", () => { expect(t.router.state.navigation.location?.pathname).toBe("/bar"); await AR.loaders.root.resolve("ROOT*"); await AR.loaders.bar.resolve("stuff"); - expect(A.fetcher).toEqual({ + expect(A.fetcher).toMatchObject({ data: undefined, state: "idle", formMethod: undefined, diff --git a/packages/router/router.ts b/packages/router/router.ts index 2ea4621da9..5e4a0c3405 100644 --- a/packages/router/router.ts +++ b/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 }; } From 0cf62e98980b6ba8033c3c4b31aa12d206d3ced6 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 17:52:24 -0500 Subject: [PATCH 2/5] add changeset and docs updates --- .changeset/kind-gifts-reflect.md | 5 +++++ docs/components/form.md | 14 +++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) create mode 100644 .changeset/kind-gifts-reflect.md diff --git a/.changeset/kind-gifts-reflect.md b/.changeset/kind-gifts-reflect.md new file mode 100644 index 0000000000..39f30e7e44 --- /dev/null +++ b/.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 diff --git a/docs/components/form.md b/docs/components/form.md index d634b9e858..32602ab5e0 100644 --- a/docs/components/form.md +++ b/docs/components/form.md @@ -179,11 +179,15 @@ Instructs the form to replace the current entry in the history stack, instead of ``` -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. From f3ddf85259a702daa5fb2924e9817443a6bdddda Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 17:59:18 -0500 Subject: [PATCH 3/5] Remove unintentional changes --- packages/router/__tests__/router-test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index d2e3acfaec..4d1dcf7f91 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -7124,7 +7124,7 @@ describe("a router", () => { let key = "key"; router.fetch(key, "root", "/"); - expect(router.state.fetchers.get(key)).toMatchObject({ + expect(router.state.fetchers.get(key)).toEqual({ state: "loading", formMethod: undefined, formEncType: undefined, @@ -7134,7 +7134,7 @@ describe("a router", () => { }); await dfd.resolve("DATA"); - expect(router.state.fetchers.get(key)).toMatchObject({ + expect(router.state.fetchers.get(key)).toEqual({ state: "idle", formMethod: undefined, formEncType: undefined, @@ -7626,7 +7626,7 @@ describe("a router", () => { expect(t.router.state.navigation.location?.pathname).toBe("/bar"); await AR.loaders.root.resolve("ROOT*"); await AR.loaders.bar.resolve("stuff"); - expect(A.fetcher).toMatchObject({ + expect(A.fetcher).toEqual({ data: undefined, state: "idle", formMethod: undefined, From 786a7e15a650c1249da72501e51d0768fd472c16 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 18:02:06 -0500 Subject: [PATCH 4/5] ocd code ;) --- packages/router/router.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index 5e4a0c3405..e406dd2cec 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -829,17 +829,16 @@ export function createRouter(init: RouterInit): Router { historyAction = HistoryAction.Replace; } else if (userReplace === false) { // no-op - } else if (submission != null && isMutationMethod(submission.formMethod)) { + } else if ( + submission != null && + isMutationMethod(submission.formMethod) && + submission.formAction === state.location.pathname + state.location.search + ) { // 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; - } + // location. If the user redirects to a different location from the + // action/loader this will be ignored and the redirect will be a PUSH + historyAction = HistoryAction.Replace; } let preventScrollReset = From 8e9747d9ccdf66065560ec8ba4effa77a694f1fb Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 15 Dec 2022 18:09:45 -0500 Subject: [PATCH 5/5] bundle bump --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 7e0b92f921..55693f3cd7 100644 --- a/package.json +++ b/package.json @@ -113,7 +113,7 @@ "none": "12.5 kB" }, "packages/react-router/dist/umd/react-router.production.min.js": { - "none": "14.5 kB" + "none": "15 kB" }, "packages/react-router-dom/dist/react-router-dom.production.min.js": { "none": "11 kB"