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.

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): fix <Form> submit to not break formMethod functionality #4053

Merged
merged 13 commits into from Oct 13, 2022
Merged

fix(remix-react): fix <Form> submit to not break formMethod functionality #4053

merged 13 commits into from Oct 13, 2022

Conversation

kevlened
Copy link
Contributor

Closes: #2162 and #3613 (supersedes both) – credits to @mdoury and @nrako

the <Form> component – unlike native <form> - does not respect the formmethod attribute set
on the submitter (the button submitting the form).

  • Docs N/A
  • Tests

Testing Strategy:

<Form method="post">
  <input name="eat" value="pizza" />
  <button type="submit"> Submit with GET </button>
</Form>
// should submit with a GET to `/?eat=pizza` but instead do a POST

See integration Bug-Report-Test via commit: 37b0140

test: failing test with <Form> not respecting formMethod 06333c1 Thu, 30 Jun 2022 00:33:15 +0200

Bug report integration test demonstrating how the `<Form>` component – 
unlike native `<form>` - does not respect the `formmethod` attribute set on
the submitter (the `<button>` submitting the form).

fix(remix-react): submit <Form> w/method set by submitter formmethod bfd1f88 Thu, 30 Jun 2022 00:32:22 +0200

Fix `<Form>` component by not passing the form's `method` into the
`options` when calling the `submit(target, options)` function (created with
the `useSubmit()` hook). This ensures that the option does not take 
precedence on any `formethod` set on the submitter which otherwise break the
`formmethod` functionality.

Since #3094 and #3094 the `<Form>` passes the "submitter" (if any) as the
`target` to the `submit(target, options)` (`useSubmitImpl`) which will
attempt to read the `formmethod`, `formaction` and `formenctype` attributes
on the target. Therefore, the responsability to infer which method should be
used should solely be on `useSubmitImpl` for the given
`target`.

Co-authored-by: Maxime Doury <maxime.doury@docavenue.com>

nrako and others added 3 commits June 30, 2022 00:41
Bug report integration test demonstrating how the `<Form>` component –
unlike native `<form>` - does not respect the `formmethod` attribute set
on the submitter (the `<button>` submitting the form).
Fix `<Form>` component by not passing the form's `method` into the
`options` when calling the `submit(target, options)` function (created
with the `useSubmit()` hook). This ensures that the option does not take
precedence on any `formethod` set on the submitter which otherwise break
the `formmethod` functionality.

Since #3094 and #3094 the `<Form>` passes the "submitter" (if any) as
the `target` to the `submit(target, options)` (`useSubmitImpl`) which
will attempt to read the `formmethod`, `formaction` and `formenctype`
attributes on the target. Therefore, the responsability to infer which
method should be used should solely be on `useSubmitImpl` for the given
`target`.

Co-authored-by: Maxime Doury <maxime.doury@docavenue.com>
@changeset-bot
Copy link

changeset-bot bot commented Aug 23, 2022

🦋 Changeset detected

Latest commit: 4028ab5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
remix Patch
@remix-run/react Patch
create-remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/node Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/vercel Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Aug 23, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@kevlened kevlened changed the title Fix submitter formethod Fix submitter formMethod Aug 23, 2022
@kevlened kevlened changed the title Fix submitter formMethod fix: Form component ignores formMethod overrides Aug 23, 2022
@MichaelDeBoey MichaelDeBoey changed the title fix: Form component ignores formMethod overrides fix(remix-react): fix <Form> submit to not break formMethod functionality Aug 25, 2022
integration/form-test.ts Outdated Show resolved Hide resolved
integration/form-test.ts Outdated Show resolved Hide resolved
integration/form-test.ts Outdated Show resolved Hide resolved
integration/form-test.ts Outdated Show resolved Hide resolved
kevlened and others added 5 commits August 26, 2022 07:18
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
integration/form-test.ts Outdated Show resolved Hide resolved
integration/form-test.ts Outdated Show resolved Hide resolved
@mcansh
Copy link
Collaborator

mcansh commented Sep 29, 2022

hm @kevlened, I just cloned this locally and checked out 06333c1, but all the tests passed for me

@kevlened
Copy link
Contributor Author

kevlened commented Oct 5, 2022

That’s odd. I’m confident the functionality is broken, but I was able to work around it. I’ll create a repro example, then try to see why the tests aren’t catching it.

@evanstern
Copy link

I can also confirm that this issue still occurs for me in the current release.

@mian-muhammad
Copy link

I am facing the same issue in the current release.

@machour machour linked an issue Oct 10, 2022 that may be closed by this pull request
@mcansh mcansh self-requested a review October 11, 2022 18:03
mcansh added a commit that referenced this pull request Oct 11, 2022
…orm.method

options.method defaults to get so will always take precedent currently

closes #4053

Signed-off-by: Logan McAnsh <logan@mcan.sh>
@kiliman
Copy link
Collaborator

kiliman commented Oct 11, 2022

Until PR #4053 is merged in, you can use patch-package. It's literally a single line change. Copy this patch to your patches folder.

https://gist.github.com/kiliman/5a8dac031da8d545dc293fdcf82e9135

@mcansh
Copy link
Collaborator

mcansh commented Oct 11, 2022

Until PR #4053 is merged in, you can use patch-package. It's literally a single line change. Copy this patch to your patches folder.

https://gist.github.com/kiliman/5a8dac031da8d545dc293fdcf82e9135

there's two solutions to this problem, the one provided by this PR and this. the issue is due to defaulting Form to method=get, which we should, however we check for options.method (and friends) lower in the component where options.method is always set as it has a default value in Form. I still lean toward the fix in #4053, but I'll leave it up to discussion

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-c49bcaf-20221014 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remix's <Form> doesn't respect <button> formMethod attribute
7 participants