From 089f4174387f461a8d8ca7064075a24032902a90 Mon Sep 17 00:00:00 2001 From: Ionut Botizan Date: Tue, 17 May 2022 15:28:25 +0300 Subject: [PATCH 1/4] bug(remix-react): add failing test for default Form action --- contributors.yml | 1 + integration/bug-report-test.ts | 43 ++++++++++------------------------ 2 files changed, 14 insertions(+), 30 deletions(-) diff --git a/contributors.yml b/contributors.yml index 280d0143a7f..4ac93533057 100644 --- a/contributors.yml +++ b/contributors.yml @@ -138,6 +138,7 @@ - ianduvall - illright - imzshh +- ionut-botizan - isaacrmoreno - ishan-me - IshanKBG diff --git a/integration/bug-report-test.ts b/integration/bug-report-test.ts index b393ddc3cb0..d6129858007 100644 --- a/integration/bug-report-test.ts +++ b/integration/bug-report-test.ts @@ -1,6 +1,6 @@ import { test, expect } from "@playwright/test"; -import { PlaywrightFixture } from "./helpers/playwright-fixture"; +import { getElement, PlaywrightFixture } from "./helpers/playwright-fixture"; import type { Fixture, AppFixture } from "./helpers/create-fixture"; import { createAppFixture, createFixture, js } from "./helpers/create-fixture"; @@ -47,30 +47,19 @@ test.beforeAll(async () => { // `createFixture` will make an app and run your tests against it. //////////////////////////////////////////////////////////////////////////// files: { - "app/routes/index.jsx": js` - import { json } from "@remix-run/node"; - import { useLoaderData, Link } from "@remix-run/react"; + "app/routes/login.jsx": js` + import { Form } from "@remix-run/react"; - export function loader() { - return json("pizza"); - } - - export default function Index() { - let data = useLoaderData(); + export default function Login() { return (
- {data} - Other Route +
+ +
) } `, - - "app/routes/burgers.jsx": js` - export default function Index() { - return
cheeseburger
; - } - `, }, }); @@ -85,22 +74,16 @@ test.afterAll(async () => appFixture.close()); // add a good description for what you expect Remix to do 👇🏽 //////////////////////////////////////////////////////////////////////////////// -test("[description of what you expect it to do]", async ({ page }) => { +test("default form action matches current path & search", async ({ page }) => { let app = new PlaywrightFixture(appFixture, page); - // You can test any request your app might get using `fixture`. - let response = await fixture.requestDocument("/"); - expect(await response.text()).toMatch("pizza"); + let url = "/login?redirectTo=/profile"; - // If you need to test interactivity use the `app` - await app.goto("/"); - await app.clickLink("/burgers"); - expect(await app.getHtml()).toMatch("cheeseburger"); + await app.goto(url); - // If you're not sure what's going on, you can "poke" the app, it'll - // automatically open up in your browser for 20 seconds, so be quick! - // await app.poke(20); + let html = await app.getHtml(); + let form = getElement(html, `form`); - // Go check out the other tests to see what else you can do. + expect(form.attr("action")).toEqual(url); }); //////////////////////////////////////////////////////////////////////////////// From 23f8191797e42bdfabad95e0eba2c5341848ac2b Mon Sep 17 00:00:00 2001 From: Ionut Botizan Date: Tue, 17 May 2022 16:34:06 +0300 Subject: [PATCH 2/4] fix(remix-react): Handle search params in form action (#3133) Preserve the current location's search params when the default Form action is used. --- integration/form-test.ts | 41 +++++++++++++++++++++++++++++ packages/remix-react/components.tsx | 5 ++++ 2 files changed, 46 insertions(+) diff --git a/integration/form-test.ts b/integration/form-test.ts index ab7cfd5de58..1a0f7f0eb0e 100644 --- a/integration/form-test.ts +++ b/integration/form-test.ts @@ -40,6 +40,8 @@ test.describe("Forms", () => { let SPLAT_ROUTE_CURRENT_ACTION = "splat-route-cur"; let SPLAT_ROUTE_PARENT_ACTION = "splat-route-parent"; let SPLAT_ROUTE_TOO_MANY_DOTS_ACTION = "splat-route-too-many-dots"; + let DEFAULT_ACTION_SEARCH_PARAMS = "default-action-search-params"; + let EXPLICIT_ACTION_SEARCH_PARAMS = "explicit-action-search-params"; test.beforeAll(async () => { fixture = await createFixture({ @@ -277,6 +279,22 @@ test.describe("Forms", () => { ) } `, + + "app/routes/login.jsx": js` + import { Form } from "@remix-run/react"; + export default function() { + return ( + <> +
+ +
+
+ +
+ + ) + } + `, }, }); @@ -574,5 +592,28 @@ test.describe("Forms", () => { expect(el.attr("action")).toMatch("/"); }); }); + + test.describe("in a route with search params", () => { + test("default action preserves current location's search params", async ({ + page, + }) => { + let app = new PlaywrightFixture(appFixture, page); + let url = "/login?redirectTo=/profile"; + await app.goto(url); + let html = await app.getHtml(); + let el = getElement(html, `#${DEFAULT_ACTION_SEARCH_PARAMS}`); + expect(el.attr("action")).toMatch(url); + }); + + test("explicit action is preserved as defined", async ({ page }) => { + let app = new PlaywrightFixture(appFixture, page); + let url = "/login?redirectTo=/profile"; + let action = "/auth?nonce=123"; + await app.goto(url); + let html = await app.getHtml(); + let el = getElement(html, `#${EXPLICIT_ACTION_SEARCH_PARAMS}`); + expect(el.attr("action")).toMatch(action); + }); + }); }); }); diff --git a/packages/remix-react/components.tsx b/packages/remix-react/components.tsx index 721e5a49ae3..d492c6767df 100644 --- a/packages/remix-react/components.tsx +++ b/packages/remix-react/components.tsx @@ -994,9 +994,14 @@ export function useFormAction( ): string { let { id } = useRemixRouteContext(); let path = useResolvedPath(action); + let location = useLocation(); let search = path.search; let isIndexRoute = id.endsWith("/index"); + if (action === ".") { + search = location.search; + } + if (action === "." && isIndexRoute) { search = search ? search.replace(/^\?/, "?index&") : "?index"; } From 57a863467292c9b5f3445568cae183cdf23dbd5d Mon Sep 17 00:00:00 2001 From: Ionut Botizan Date: Sat, 11 Jun 2022 21:45:23 +0300 Subject: [PATCH 3/4] fix(remix-react): Preserve search only for undefined form actions --- integration/form-test.ts | 8 ++++---- packages/remix-react/components.tsx | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/integration/form-test.ts b/integration/form-test.ts index 1a0f7f0eb0e..11032fa88e3 100644 --- a/integration/form-test.ts +++ b/integration/form-test.ts @@ -40,7 +40,7 @@ test.describe("Forms", () => { let SPLAT_ROUTE_CURRENT_ACTION = "splat-route-cur"; let SPLAT_ROUTE_PARENT_ACTION = "splat-route-parent"; let SPLAT_ROUTE_TOO_MANY_DOTS_ACTION = "splat-route-too-many-dots"; - let DEFAULT_ACTION_SEARCH_PARAMS = "default-action-search-params"; + let UNDEFINED_ACTION_SEARCH_PARAMS = "undefined-action-search-params"; let EXPLICIT_ACTION_SEARCH_PARAMS = "explicit-action-search-params"; test.beforeAll(async () => { @@ -285,7 +285,7 @@ test.describe("Forms", () => { export default function() { return ( <> -
+
@@ -594,14 +594,14 @@ test.describe("Forms", () => { }); test.describe("in a route with search params", () => { - test("default action preserves current location's search params", async ({ + test("undefined action preserves current location's search params", async ({ page, }) => { let app = new PlaywrightFixture(appFixture, page); let url = "/login?redirectTo=/profile"; await app.goto(url); let html = await app.getHtml(); - let el = getElement(html, `#${DEFAULT_ACTION_SEARCH_PARAMS}`); + let el = getElement(html, `#${UNDEFINED_ACTION_SEARCH_PARAMS}`); expect(el.attr("action")).toMatch(url); }); diff --git a/packages/remix-react/components.tsx b/packages/remix-react/components.tsx index d492c6767df..6f4ec7ca4c7 100644 --- a/packages/remix-react/components.tsx +++ b/packages/remix-react/components.tsx @@ -933,7 +933,7 @@ let FormImpl = React.forwardRef( reloadDocument = false, replace = false, method = "get", - action = ".", + action, encType = "application/x-www-form-urlencoded", fetchKey, onSubmit, @@ -988,21 +988,21 @@ 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 path = useResolvedPath(action ?? "."); let location = useLocation(); let search = path.search; let isIndexRoute = id.endsWith("/index"); - if (action === ".") { + if (action === undefined) { search = location.search; } - if (action === "." && isIndexRoute) { + if ((action === undefined || action === ".") && isIndexRoute) { search = search ? search.replace(/^\?/, "?index&") : "?index"; } From d94856d524429918083a1827bbfc1c972e866e3e Mon Sep 17 00:00:00 2001 From: Ionut Botizan Date: Sat, 11 Jun 2022 22:07:30 +0300 Subject: [PATCH 4/4] fix(remix-react): Revert changes to the bug-report-test template Now that the fix has been applied, the passing tests have been moved to `integration/form-test` and the template has been reverted. --- integration/bug-report-test.ts | 43 ++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/integration/bug-report-test.ts b/integration/bug-report-test.ts index d6129858007..b393ddc3cb0 100644 --- a/integration/bug-report-test.ts +++ b/integration/bug-report-test.ts @@ -1,6 +1,6 @@ import { test, expect } from "@playwright/test"; -import { getElement, PlaywrightFixture } from "./helpers/playwright-fixture"; +import { PlaywrightFixture } from "./helpers/playwright-fixture"; import type { Fixture, AppFixture } from "./helpers/create-fixture"; import { createAppFixture, createFixture, js } from "./helpers/create-fixture"; @@ -47,19 +47,30 @@ test.beforeAll(async () => { // `createFixture` will make an app and run your tests against it. //////////////////////////////////////////////////////////////////////////// files: { - "app/routes/login.jsx": js` - import { Form } from "@remix-run/react"; + "app/routes/index.jsx": js` + import { json } from "@remix-run/node"; + import { useLoaderData, Link } from "@remix-run/react"; - export default function Login() { + export function loader() { + return json("pizza"); + } + + export default function Index() { + let data = useLoaderData(); return (
- - - + {data} + Other Route
) } `, + + "app/routes/burgers.jsx": js` + export default function Index() { + return
cheeseburger
; + } + `, }, }); @@ -74,16 +85,22 @@ test.afterAll(async () => appFixture.close()); // add a good description for what you expect Remix to do 👇🏽 //////////////////////////////////////////////////////////////////////////////// -test("default form action matches current path & search", async ({ page }) => { +test("[description of what you expect it to do]", async ({ page }) => { let app = new PlaywrightFixture(appFixture, page); - let url = "/login?redirectTo=/profile"; + // You can test any request your app might get using `fixture`. + let response = await fixture.requestDocument("/"); + expect(await response.text()).toMatch("pizza"); - await app.goto(url); + // If you need to test interactivity use the `app` + await app.goto("/"); + await app.clickLink("/burgers"); + expect(await app.getHtml()).toMatch("cheeseburger"); - let html = await app.getHtml(); - let form = getElement(html, `form`); + // If you're not sure what's going on, you can "poke" the app, it'll + // automatically open up in your browser for 20 seconds, so be quick! + // await app.poke(20); - expect(form.attr("action")).toEqual(url); + // Go check out the other tests to see what else you can do. }); ////////////////////////////////////////////////////////////////////////////////