Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(remix-react): Preserve search string in form action when action prop is omitted #3697

Merged
merged 15 commits into from
Aug 10, 2022
Merged

fix(remix-react): Preserve search string in form action when action prop is omitted #3697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4f907c1
chore(tests): add test on gists app (#927)
jacargentina Dec 6, 2021
70a1115
Merge branch 'dev' into dev
jacargentina Jan 25, 2022
7cbbad3
Merge upstream
chaance Jul 10, 2022
291f3cc
update tests
chaance Jul 11, 2022
74b057d
fix: preserve search params w/ empty form action
chaance Jul 11, 2022
6d2cd40
add changeset
chaance Jul 11, 2022
eadf103
Merge branch 'dev' into chance/use-form-action-fix
chaance Aug 3, 2022
0be6f72
update tests
chaance Aug 3, 2022
783c435
differentiate between no action and '.'
chaance Aug 3, 2022
e12927b
update changeset
chaance Aug 3, 2022
b9414ab
update test assertions
chaance Aug 3, 2022
f6ddd78
use `createPath` instead of string concat
chaance Aug 4, 2022
0409401
Merge branch 'dev' into chance/use-form-action-fix
chaance Aug 10, 2022
bf9e435
check explicitly for nullish action
chaance Aug 10, 2022
9147639
use nullish check instead of explicit undefined check
chaance Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/curvy-drinks-flash.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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");
chaance marked this conversation as resolved.
Show resolved Hide resolved
});

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