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

Add support for FormData request body #1835

Merged
merged 15 commits into from Aug 21, 2021

Conversation

octet-stream
Copy link
Contributor

@octet-stream octet-stream commented Aug 12, 2021

This PR brings general support for multipart/form-data encoding and allows to use any spec-comliant FormData object as request body. This PR also introduces a new dependency form-data-encoder, so you might expect it to add about 37 KB of extra size into the install size of got.

Before it's done I would like to get a feedback on few more things:

  • How should I deal with custom content-type header? The encoder will generate this header and use internally to produce body content and if user will override this header - body will not be send as of now. Should I create and use the encoder in request body anyway?
  • Do I need some more tests? Perhaps for request body? The encoder.encode() method returns AsyncIterableIterator so it basically reuse the logic for async iterator body consuming.
  • How would I document these changes? Should I add alternative packages (both formdata-node and formdata-polyfill) into the docs?

I will keep this PR as WIP until my questions is resolved.

Further suggestions: As I said before, the good old form-data package is not spec-compliant and I think it should be deprecated. At least node-fetch will do so in near future (see refs section of this PR for more information).

Refs:

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates in both the README and the types.

@octet-stream octet-stream changed the title Implement general support for FormData request body using form-data-encoder. Implement general support for FormData request body. Aug 12, 2021
@octet-stream
Copy link
Contributor Author

Ah, I forgot to mention that while tests are passing, I have issues with XO. But they not seem to be related to this PR.

@szmarczak
Copy link
Collaborator

Thanks for the draft! Looking forward to this ^_^

How should I deal with custom content-type header? The encoder will generate this header and use internally to produce body content and if user will override this header - body will not be send as of now.

It should be sent no matter the content type.

Do I need some more tests? Perhaps for request body?

You read my mind :) Basically read the payload on the server and check if it's valid.

How would I document these changes? Should I add alternative packages (both formdata-node and formdata-polyfill) into the docs?

👍

the good old form-data package is not spec-compliant and I think it should be deprecated.

I agree.

/cc @sindresorhus

@sindresorhus
Copy link
Owner

👍

This will also need documentation updates.

@octet-stream
Copy link
Contributor Author

octet-stream commented Aug 12, 2021

It should be sent no matter the content type.

So I should keep header set by developer and not override it. Right?

You read my mind :) Basically read the payload on the server and check if it's valid.

👍

I agree.

Should I include form-data deprecation in this PR? If so, how should I mention this? With util.deprecate or/and documentation? I'm asking in case if got has some guidelines for this case :)

This will also need documentation updates.

Will do!

@szmarczak
Copy link
Collaborator

So I should keep header set by developer and not override it. Right?

Correct. Only set content-type if noContentType.

With util.deprecate or/and documentation?

👍 (and)

Should I include form-data deprecation in this PR?

To be honest, I'm not sure. I'm leaving this up to @sindresorhus

…ontent-type header provided by form-data-encoder only if this header is not set by a developer. Mention spec-compliant FormData implementations in documentation.
@octet-stream
Copy link
Contributor Author

Hmm. Do you guys have a body parser to handle FomrData bodies in your test server?

Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
@szmarczak
Copy link
Collaborator

Do you guys have a body parser to handle FomrData bodies in your test server?

We don't. Feel free to include one.

@octet-stream
Copy link
Contributor Author

About then-busboy: To be honest, I am not familiar with express ecosystem and if there's any multipart/form-data parser that can produce a single object with files and fields. I just needed a parser that will help be to get files, read them and send back along with the fields. Maybe multer can handle this? It seems like they allow to upload only files with specific field names (only those you mention in .array(), .fields() and .single()), otherwise if you will use multer.any() you will get files as an array.

I was ended up trying to make a bare minimal parser, but realised that I basically re-implement my solution... Sooo I just added it for tests. It seem to work as expected.

@octet-stream
Copy link
Contributor Author

I think I gonna mark this PR as ready for a review and wait for your feedback, because I do not have any more significant changes for this feature.

@octet-stream octet-stream marked this pull request as ready for review August 13, 2021 00:26
@octet-stream
Copy link
Contributor Author

octet-stream commented Aug 13, 2021

Looks like I have fixed XO errors related to this PR.

For some reason I also have these errors on my local machine.

image

But they don't seem to be a thing in previous run of CI workflow.

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@sindresorhus sindresorhus changed the title Implement general support for FormData request body. Add support for FormData request body Aug 16, 2021
@octet-stream
Copy link
Contributor Author

I think this tests failing is not related to the PR...

@szmarczak szmarczak merged commit 0fb6ec6 into sindresorhus:main Aug 21, 2021
@szmarczak
Copy link
Collaborator

Big thank you @octet-stream ❤️

@octet-stream octet-stream deleted the feature/form-data-encoding branch August 21, 2021 15:20
@octet-stream
Copy link
Contributor Author

It was a pleasure :)

@tutods
Copy link

tutods commented Oct 16, 2022

Is possible now use Got with multipart/formdata?

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

Successfully merging this pull request may close these issues.

None yet

4 participants