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

[Bug]: FormEncType type is missing newly supported application/json #10646

Closed
nicksrandall opened this issue Jun 26, 2023 · 7 comments
Closed
Labels

Comments

@nicksrandall
Copy link

What version of React Router are you using?

6.14

Steps to Reproduce

Try to use Fetcher.submit in a TS environment with encType: "application/json"

Expected Behavior

New encTypes would be allowed by TS

Actual Behavior

TS complains because types are missing.

nicksrandall added a commit to nicksrandall/react-router that referenced this issue Jun 26, 2023
@nicksrandall nicksrandall closed this as not planned Won't fix, can't repro, duplicate, stale Jun 26, 2023
@kiliman
Copy link
Contributor

kiliman commented Jun 27, 2023

Interesting. It's explicitly suggested in the Remix v1.18 release notes, so either the note is wrong, or the type wasn't updated as needed.

image

https://github.com/remix-run/remix/releases/tag/remix%401.18.0

@timdorr
Copy link
Member

timdorr commented Jun 27, 2023

I'm wondering if that's a design error on Remix's part. The spec only lists three possible values for encType, so application/json would be erroneous according to the spec and therefore be invalid HTML when rendered.

But at the same time, the application/* MIME types are generally intended for application-specific definitions outside of hard specification values. And handling JSON-formatted data would be helpful for a number of different reasons.

There's probably a middle ground here. I'm not sure if the discussion should live here or the Remix repo. But it's probably worth discussing.

@kiliman
Copy link
Contributor

kiliman commented Jun 27, 2023

I don't think it's a spec issue. The whole point of allowing submit and fetcher.submit to post JSON data is that the native API doesn't support it. This is an extension that explicitly enables that use case.

Technically I think it should allow any encType. For known types like application/x-www-form-urlencoded and application/json, the function will know how to convert the body. For anything else, it should just set the body and the Content-Type header of the request. The action code can check the request header and parse the body accordingly.

@brophdawg11
Copy link
Contributor

@nicksrandall Can you double check your local setup and/or provide a reproduction?

FormEncType was updated in 6.14.0 to include all 4 options, and both useSubmit and fetcher.submit should support it via SubmitOptions.

Locally this works as expected for me using 6.14.0:

Screenshot 2023-06-29 at 9 50 48 AM

As noted above, application/json is not supported by <form>, and thus is not allowed on the <Form> component.

@brophdawg11
Copy link
Contributor

Technically I think it should allow any encType

@kiliman That's not a bad idea, and would be pretty minor changes to allow - give us a string body and we'll just send whatever encType you specified and let you work it out via request.text(). Feel free to open a proposal and we can run it by the rest of the team.

@nicksrandall
Copy link
Author

I closed this issue because I realized my setup was wrong.

That said, I wish the type signature of submit was narrowed by encType.

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

No branches or pull requests

5 participants