Skip to content

Commit

Permalink
fix(remix-react): Preserve search string in form action when action…
Browse files Browse the repository at this point in the history 
… prop is omitted (#3697)

Co-authored-by: Ionut Botizan <ionut.botizan@haufe-lexware.com>
Co-authored-by: Javier Castro <javier.alejandro.castro@gmail.com>
  • Loading branch information
@chaance @jacargentina @mcansh
3 people authored and mcansh committed Aug 11, 2022
1 parent 00aec5b commit 351cb83
Show file tree
Hide file tree
Showing 4 changed files with 191 additions and 12 deletions.
5 changes: 5 additions & 0 deletions .changeset/curvy-drinks-flash.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

Previously, if an `action` was omitted from `<Form>` 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.
2 changes: 2 additions & 0 deletions contributors.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,12 @@
- ianduvall
- illright
- imzshh
- ionut-botizan
- isaacrmoreno
- ishan-me
- IshanKBG
- itsMapleLeaf
- jacargentina
- jacob-ebey
- JacobParis
- jakewtaylor
Expand Down
170 changes: 165 additions & 5 deletions integration/form-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -131,6 +136,9 @@ test.describe("Forms", () => {
export default function() {
return (
<>
<Form id="${STATIC_ROUTE_NO_ACTION}">
<button>Submit</button>
</Form>
<Form id="${STATIC_ROUTE_ABSOLUTE_ACTION}" action="/about">
<button>Submit</button>
</Form>
Expand All @@ -154,6 +162,9 @@ test.describe("Forms", () => {
return (
<>
<h1>Blog</h1>
<Form id="${LAYOUT_ROUTE_NO_ACTION}">
<button>Submit</button>
</Form>
<Form id="${LAYOUT_ROUTE_ABSOLUTE_ACTION}" action="/about">
<button>Submit</button>
</Form>
Expand All @@ -177,6 +188,9 @@ test.describe("Forms", () => {
export default function() {
return (
<>
<Form id="${INDEX_ROUTE_NO_ACTION}">
<button>Submit</button>
</Form>
<Form id="${INDEX_ROUTE_ABSOLUTE_ACTION}" action="/about">
<button>Submit</button>
</Form>
Expand All @@ -199,6 +213,9 @@ test.describe("Forms", () => {
export default function() {
return (
<>
<Form id="${DYNAMIC_ROUTE_NO_ACTION}">
<button>Submit</button>
</Form>
<Form id="${DYNAMIC_ROUTE_ABSOLUTE_ACTION}" action="/about">
<button>Submit</button>
</Form>
Expand Down Expand Up @@ -239,6 +256,9 @@ test.describe("Forms", () => {
export default function() {
return (
<>
<Form id="${SPLAT_ROUTE_NO_ACTION}">
<button>Submit</button>
</Form>
<Form id="${SPLAT_ROUTE_ABSOLUTE_ACTION}" action="/about">
<button>Submit</button>
</Form>
Expand Down Expand Up @@ -389,6 +409,26 @@ test.describe("Forms", () => {

test.describe("<Form> 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,
}) => {
Expand All @@ -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);
Expand All @@ -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,
}) => {
Expand All @@ -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,
}) => {
Expand All @@ -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);
Expand All @@ -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,
}) => {
Expand All @@ -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,
}) => {
Expand All @@ -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);
Expand All @@ -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,
}) => {
Expand All @@ -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,
}) => {
Expand All @@ -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);
Expand All @@ -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,
}) => {
Expand All @@ -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,
}) => {
Expand All @@ -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);
Expand All @@ -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,
}) => {
Expand Down
Loading

0 comments on commit 351cb83

Please sign in to comment.