From 7d042b85ab18dbe2518daf07379c88e51123a6b7 Mon Sep 17 00:00:00 2001 From: Chance Strickland Date: Wed, 10 Aug 2022 13:35:58 -0700 Subject: [PATCH] fix(remix-react): Preserve search string in form action when `action` prop is omitted (#3697) Co-authored-by: Ionut Botizan Co-authored-by: Javier Castro --- .changeset/curvy-drinks-flash.md | 5 + contributors.yml | 2 + integration/form-test.ts | 170 +++++++++++++++++++++++++++- packages/remix-react/components.tsx | 26 +++-- 4 files changed, 191 insertions(+), 12 deletions(-) create mode 100644 .changeset/curvy-drinks-flash.md diff --git a/.changeset/curvy-drinks-flash.md b/.changeset/curvy-drinks-flash.md new file mode 100644 index 00000000000..24b78d03bbb --- /dev/null +++ b/.changeset/curvy-drinks-flash.md @@ -0,0 +1,5 @@ +--- +"@remix-run/react": patch +--- + +Previously, if an `action` was omitted from `
` or `useFormAction`, the action value would default to `"."`. This is incorrect, as `"."` should resolve based on the current _path_, but an empty action resolves relative to the current _URL_ (including the search and hash values). We've fixed this to differentiate between the two. diff --git a/contributors.yml b/contributors.yml index 205886e7c6f..4c75b792f2d 100644 --- a/contributors.yml +++ b/contributors.yml @@ -158,10 +158,12 @@ - ianduvall - illright - imzshh +- ionut-botizan - isaacrmoreno - ishan-me - IshanKBG - itsMapleLeaf +- jacargentina - jacob-ebey - JacobParis - jakewtaylor diff --git a/integration/form-test.ts b/integration/form-test.ts index 26772b65cd6..e66c2e0d9f9 100644 --- a/integration/form-test.ts +++ b/integration/form-test.ts @@ -20,22 +20,27 @@ test.describe("Forms", () => { let ACTION = "action"; let EAT = "EAT"; + let STATIC_ROUTE_NO_ACTION = "static-route-none"; let STATIC_ROUTE_ABSOLUTE_ACTION = "static-route-abs"; let STATIC_ROUTE_CURRENT_ACTION = "static-route-cur"; let STATIC_ROUTE_PARENT_ACTION = "static-route-parent"; let STATIC_ROUTE_TOO_MANY_DOTS_ACTION = "static-route-too-many-dots"; + let INDEX_ROUTE_NO_ACTION = "index-route-none"; let INDEX_ROUTE_ABSOLUTE_ACTION = "index-route-abs"; let INDEX_ROUTE_CURRENT_ACTION = "index-route-cur"; let INDEX_ROUTE_PARENT_ACTION = "index-route-parent"; let INDEX_ROUTE_TOO_MANY_DOTS_ACTION = "index-route-too-many-dots"; + let DYNAMIC_ROUTE_NO_ACTION = "dynamic-route-none"; let DYNAMIC_ROUTE_ABSOLUTE_ACTION = "dynamic-route-abs"; let DYNAMIC_ROUTE_CURRENT_ACTION = "dynamic-route-cur"; let DYNAMIC_ROUTE_PARENT_ACTION = "dynamic-route-parent"; let DYNAMIC_ROUTE_TOO_MANY_DOTS_ACTION = "dynamic-route-too-many-dots"; + let LAYOUT_ROUTE_NO_ACTION = "layout-route-none"; let LAYOUT_ROUTE_ABSOLUTE_ACTION = "layout-route-abs"; let LAYOUT_ROUTE_CURRENT_ACTION = "layout-route-cur"; let LAYOUT_ROUTE_PARENT_ACTION = "layout-route-parent"; let LAYOUT_ROUTE_TOO_MANY_DOTS_ACTION = "layout-route-too-many-dots"; + let SPLAT_ROUTE_NO_ACTION = "splat-route-none"; let SPLAT_ROUTE_ABSOLUTE_ACTION = "splat-route-abs"; let SPLAT_ROUTE_CURRENT_ACTION = "splat-route-cur"; let SPLAT_ROUTE_PARENT_ACTION = "splat-route-parent"; @@ -131,6 +136,9 @@ test.describe("Forms", () => { export default function() { return ( <> + + +
@@ -154,6 +162,9 @@ test.describe("Forms", () => { return ( <>

Blog

+
+ +
@@ -177,6 +188,9 @@ test.describe("Forms", () => { export default function() { return ( <> +
+ +
@@ -199,6 +213,9 @@ test.describe("Forms", () => { export default function() { return ( <> +
+ +
@@ -239,6 +256,9 @@ test.describe("Forms", () => { export default function() { return ( <> +
+ +
@@ -389,6 +409,26 @@ test.describe("Forms", () => { test.describe("
action", () => { test.describe("in a static route", () => { + test("no action resolves relative to the closest route", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/inbox"); + let html = await app.getHtml(); + let el = getElement(html, `#${STATIC_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/inbox"); + }); + + test("no action resolves to URL including search params", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/inbox?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${STATIC_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/inbox?foo=bar"); + }); + test("absolute action resolves relative to the root route", async ({ page, }) => { @@ -399,7 +439,7 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/about"); }); - test("'.' action resolves relative to the current route", async ({ + test("'.' action resolves relative to the closest route", async ({ page, }) => { let app = new PlaywrightFixture(appFixture, page); @@ -409,6 +449,14 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/inbox"); }); + test("'.' excludes search params", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/inbox?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${STATIC_ROUTE_CURRENT_ACTION}`); + expect(el.attr("action")).toMatch("/inbox"); + }); + test("'..' action resolves relative to the parent route", async ({ page, }) => { @@ -431,6 +479,26 @@ test.describe("Forms", () => { }); test.describe("in a dynamic route", () => { + test("no action resolves relative to the closest route", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/blog/abc"); + let html = await app.getHtml(); + let el = getElement(html, `#${DYNAMIC_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/blog/abc"); + }); + + test("no action resolves to URL including search params", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/blog/abc?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${DYNAMIC_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/blog/abc?foo=bar"); + }); + test("absolute action resolves relative to the root route", async ({ page, }) => { @@ -441,7 +509,7 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/about"); }); - test("'.' action resolves relative to the current route", async ({ + test("'.' action resolves relative to the closest route", async ({ page, }) => { let app = new PlaywrightFixture(appFixture, page); @@ -451,6 +519,14 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/blog/abc"); }); + test("'.' excludes search params", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/blog/abc?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${DYNAMIC_ROUTE_CURRENT_ACTION}`); + expect(el.attr("action")).toMatch("/blog/abc"); + }); + test("'..' action resolves relative to the parent route", async ({ page, }) => { @@ -473,6 +549,26 @@ test.describe("Forms", () => { }); test.describe("in an index route", () => { + test("no action resolves relative to the closest route", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/blog"); + let html = await app.getHtml(); + let el = getElement(html, `#${INDEX_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/blog"); + }); + + test("no action resolves to URL including search params", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/blog?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${INDEX_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/blog?index&foo=bar"); + }); + test("absolute action resolves relative to the root route", async ({ page, }) => { @@ -483,7 +579,7 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/about"); }); - test("'.' action resolves relative to the current route", async ({ + test("'.' action resolves relative to the closest route", async ({ page, }) => { let app = new PlaywrightFixture(appFixture, page); @@ -493,6 +589,14 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/blog"); }); + test("'.' excludes search params", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/blog?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${INDEX_ROUTE_CURRENT_ACTION}`); + expect(el.attr("action")).toMatch("/blog"); + }); + test("'..' action resolves relative to the parent route", async ({ page, }) => { @@ -515,6 +619,26 @@ test.describe("Forms", () => { }); test.describe("in a layout route", () => { + test("no action resolves relative to the closest route", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/blog"); + let html = await app.getHtml(); + let el = getElement(html, `#${LAYOUT_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/blog"); + }); + + test("no action resolves to URL including search params", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/blog?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${LAYOUT_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/blog?foo=bar"); + }); + test("absolute action resolves relative to the root route", async ({ page, }) => { @@ -525,7 +649,7 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/about"); }); - test("'.' action resolves relative to the current route", async ({ + test("'.' action resolves relative to the closest route", async ({ page, }) => { let app = new PlaywrightFixture(appFixture, page); @@ -535,6 +659,14 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/blog"); }); + test("'.' excludes search params", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/blog?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${LAYOUT_ROUTE_CURRENT_ACTION}`); + expect(el.attr("action")).toMatch("/blog"); + }); + test("'..' action resolves relative to the parent route", async ({ page, }) => { @@ -557,6 +689,26 @@ test.describe("Forms", () => { }); test.describe("in a splat route", () => { + test("no action resolves relative to the closest route", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/projects/blarg"); + let html = await app.getHtml(); + let el = getElement(html, `#${SPLAT_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/projects"); + }); + + test("no action resolves to URL including search params", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/projects/blarg?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${SPLAT_ROUTE_NO_ACTION}`); + expect(el.attr("action")).toMatch("/projects?foo=bar"); + }); + test("absolute action resolves relative to the root route", async ({ page, }) => { @@ -567,7 +719,7 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/about"); }); - test("'.' action resolves relative to the current route", async ({ + test("'.' action resolves relative to the closest route", async ({ page, }) => { let app = new PlaywrightFixture(appFixture, page); @@ -577,6 +729,14 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/projects"); }); + test("'.' excludes search params", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/projects/blarg?foo=bar"); + let html = await app.getHtml(); + let el = getElement(html, `#${SPLAT_ROUTE_CURRENT_ACTION}`); + expect(el.attr("action")).toMatch("/projects"); + }); + test("'..' action resolves relative to the parent route", async ({ page, }) => { diff --git a/packages/remix-react/components.tsx b/packages/remix-react/components.tsx index 67c68d26f27..7b89d3d2332 100644 --- a/packages/remix-react/components.tsx +++ b/packages/remix-react/components.tsx @@ -21,6 +21,7 @@ import { } from "react-router-dom"; import type { LinkProps, NavLinkProps } from "react-router-dom"; import type { Merge } from "type-fest"; +import { createPath } from "history"; import type { AppData, FormEncType, FormMethod } from "./data"; import type { EntryContext, AssetsManifest } from "./entry"; @@ -944,7 +945,7 @@ let FormImpl = React.forwardRef( reloadDocument = false, replace = false, method = "get", - action = ".", + action, encType = "application/x-www-form-urlencoded", fetchKey, onSubmit, @@ -999,20 +1000,31 @@ type HTMLFormSubmitter = HTMLButtonElement | HTMLInputElement; * @see https://remix.run/api/remix#useformaction */ export function useFormAction( - action = ".", + action?: string, // TODO: Remove method param in v2 as it's no longer needed and is a breaking change method: FormMethod = "get" ): string { let { id } = useRemixRouteContext(); - let path = useResolvedPath(action); - let search = path.search; - let isIndexRoute = id.endsWith("/index"); + let resolvedPath = useResolvedPath(action ?? "."); + + // Previously we set the default action to ".". The problem with this is that + // `useResolvedPath(".")` excludes search params and the hash of the resolved + // URL. This is the intended behavior of when "." is specifically provided as + // the form action, but inconsistent w/ browsers when the action is omitted. + // https://github.com/remix-run/remix/issues/927 + let location = useLocation(); + let { search, hash } = resolvedPath; + if (action == null) { + search = location.search; + hash = location.hash; + } - if (action === "." && isIndexRoute) { + let isIndexRoute = id.endsWith("/index"); + if ((action == null || action === ".") && isIndexRoute) { search = search ? search.replace(/^\?/, "?index&") : "?index"; } - return path.pathname + search; + return createPath({ pathname: resolvedPath.pathname, search, hash }); } export interface SubmitOptions {