fix(remix-react): Form's method shouldn't be ignored

[maxschwarzmueller]

Fixes #4412

[changeset-bot]

⚠️ No Changeset found

Latest commit: d9762d6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[remix-cla-bot]

Hi @maxschwarzmueller,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot]

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

[machour]

Thank you so much for the reproduction and fix @maxschwarzmueller !
Could you remove the generated file?
I'll get eyes on this PR today hopefully

problem fixed

[jenseng]

The spec only supports POST and GET, so this change will make <Form> behave inconsistently with <form>. As a consequence, <Form> submissions may break if they happen before/without JavaScript.

[maxschwarzmueller]

The spec only supports POST and GET, so this change will make <Form> behave inconsistently with <form>. As a consequence, <Form> submissions may break if they happen before/without JavaScript.

I'm aware of the <form> default and spec. This is not a feature request but a bug fix as Remix documentation clearly states that Remix <Form> supports DELETE, PATCH etc. in addition to GET& POST: https://remix.run/docs/en/v1/api/remix#form-method

Or am I missing something here?

[machour]

@jenseng this is stressed out in the docs (https://remix.run/docs/en/v1/api/remix#form-method) but I'd love to see <Form> forbid anything that's not get or post, even when JS is running properly.

[maxschwarzmueller]

I'll leave that up to you.
Personally, I see it as a huge plus that Remix supports DELETE etc. for <Form>. It's easy enough to stick to POST/GET and / or <form> if you want full spec-compliance + "no JS"-support.
This PR simply provides a fix to go back to the intended behavior. If it's not the wanted behavior, the docs should be updated.

[chaance]

We definitely shouldn't break existing behavior, so limiting <Form> to GET and POST would need to be considered in a future major release. Probably worth opening a discussion for that if that's something you feel strongly about, as we have no plans for that at the moment. This is an intentional deviation from the basic <form> as noted in the docs. The no-JS tests should reflect those differences for now.

[jenseng]

Sounds like there are two things going on here... if <Form method> or <button formMethod> are ignored, then that is buggy and should be fixed. Though I'm curious how it's broken without this change, as I think this logic looks correct, and the prior version of the uses the form method attribute test seems to be checking it, though I might be missing something here. EDIT: the logic I linked is wrong since it's looking at the form element's method, not the <Form>'s method prop.

As for other methods, those are great points on the docs and breaking changes. I wonder if we should more strongly discourage their usage (e.g. in dev mode do a console.warn)? One of Remix' big selling points is that it works before/without JavaScript, and this violates that.

[chaance]

As for other methods, those are great points on the docs and breaking changes. I wonder if we should more strongly discourage their usage (e.g. in dev mode do a console.warn)? One of Remix' big selling points is that it works before/without JavaScript, and this violates that.

Would you like to start a new discussion on that? I think there's merit and I'd be happy to run it up the flag pole (aka get @ryanflorence to weigh in since his GH notifications are a tire fire 😂)

[jenseng]

Sure thing, will do 👍

…

On Tue, Oct 25, 2022 at 12:52 PM Chance Strickland ***@***.***> wrote: As for other methods, those are great points on the docs and breaking changes. I wonder if we should more strongly discourage their usage (e.g. in dev mode do a console.warn)? One of Remix' big selling points is that it works before/without JavaScript, and this violates that. Would you like to start a new discussion on that? I think there's merit and I'd be happy to run it up the flag pole (aka get @ryanflorence <https://github.com/ryanflorence> to weigh in since his GH notifications are a tire fire 😂) — Reply to this email directly, view it on GitHub <#4413 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA2XMKOBUT24MVM5XQ6LS3WFAT7JANCNFSM6AAAAAARNVVQIQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jenseng]

Opened a discussion here

[chaance]

Should be fixed in 1.7.4-pre.0. We'll get a stable release out tomorrow.

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-4e8cbd8-20221027 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!