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

feat: Implement form-data encoding #603

Merged
merged 177 commits into from Jun 10, 2020

Conversation

octet-stream
Copy link
Contributor

@octet-stream octet-stream commented Apr 9, 2019

This pull request brings support of the spec-compliant FormData in request body.
As an example I'm using formdata-node package (for tests and docs).

Notes on the PR:

  • Differences between form-data and formdata-node: [QUESTION]: form-data vs formdata-node octet-stream/form-data#1
  • Content-Length is not supported for formdata-node because the package only have asynchronous method for that: FormData#getComputedLength()
  • formdata-node requires at least Node 8, but tests seem to work on Node 6 as well. I think, the PR might be delivered in 3.x if you'll drop older Node versions
  • Readable.from is used for data consumption, so the minimal Node.js version is raised to 10.17 (from 10.16)

@jimmywarting
Copy link
Collaborator

jimmywarting commented Apr 9, 2019

Just some input/feedback

If we should support any kind of formdata we should begin to implement our own way iterating over all key/values and generate the boundary/body ourself so we can support any custom formdata implementation. But form-data currently don't have that functionality 😞

I was considering making my FormData implementation able to run on node too (it's all synchronous). But it uses Blob so much that it's impractical to implement in Node.

However i think that formdata-node version looks nicer then form-data imo since it has all formdata methods and discarded the hole http request stuff. But i think it has too many other none speced functions that shouldn't be there like:

  • isFormData
    • you should check this yourself wither with toString/symbol.toStringTag or instance of
  • get boundary
  • get headers
  • stream
  • getComputedLength
  • inspect
  • asyncIterator

Also i can see that formdata-node breaks some test that my browser polyfilled version handles correctly.

I'm also a bit concerned about the constructor to, maybe in the feature FormData constructor will accept simular things like the URLSearchParams but it can also include files

I get that they some methods are necessary like some async methods, getting the size. but i think they should be wrapped in a Blob/File like object. that has the size and filename information along with it

Now how would you read the blob without a FileReader you may ask? well, the w3c/FileAPI are implementing new methods to the Blob prototype, namely blob.arrayBuffer(), blob.text() both of which returns a promise, and also blob.stream() chrome are currently implementing it now as a experimental feature

That will be the key features to how we would know how to read the hole content of a custom FormData implementation including files. (files that have not been read into the memory)

So a good formdata implementation would

  1. take the string path and read the size synchronously
  2. create a File like object with name/size and make a custom arrayBuffer(), text() and stream() implementation

for other stream you would have to create a File like object with a given size too

@jimmywarting
Copy link
Collaborator

Also can't help but thinking that you have been a bit influenced by my version cuz they looks a bit similar :)

@octet-stream
Copy link
Contributor Author

octet-stream commented Apr 9, 2019

No, I'm pretty sure I haven't been influenced by your implementation. Originally formdata-node was created because form-data does not cover FormData API (as I mentioned in first issue of the package). But yeah, I now about your implementation. And I tried it few years ago in one of my project, because Safari was like form-data package – with the only .append() method available.

Also i can see that formdata-node breaks some test that my browser polyfilled
version handles correctly.

Can you tell me bore of these cases? Maybe I missed something.

If we should support any kind of formdata we should begin to implement our own way iterating over all key/values and generate the boundary/body ourself so we can support any custom formdata implementation. But form-data currently don't have that functionality 😞

That's may be a good point, but it means that another HTTP clients might have to implement that too if they want to support FormData implementations same way. I guess.
And it also means that you have to make yet another partial implementation! :D You know, that part where you read data from form-data.

And I don't think is that having those additional methods is something bad, since they're here only to take alternative way to read the data from FormData instance. Like FormData#_blob in your implementation, I guess.

@bitinn
Copy link
Collaborator

bitinn commented Apr 13, 2019

Thx for the PR, this is definitely something we should look into on 3.x pre-release. Being modular and allow third-party to provide API implementation is one of our goal.

(To be honest I am a quite busy with my C# project, if @jimmywarting @octet-stream you would like to merge this and help move forward 3.x pre-release, I am happy to help)

@tinovyatkin
Copy link
Member

@octet-stream any idea why Windows test is failing?

@octet-stream
Copy link
Contributor Author

Not really. But this happens in main.js tests only, which means the rest of encoding implementation works fine on windows and the problem is not in formdata-node as well. Maybe the problem appears somewhere in node-fetch integration with form-data or it is the test server problem, or maybe something else.
I will test it on Windows in VM, maybe it will help me to figure out something.

@tinovyatkin
Copy link
Member

tinovyatkin commented Jun 10, 2020

@octet-stream it was an outdated dependency problem. Replacing parted with busboy fixes test on Windows

Breaking tests are irrelevant to this PR (is due to fetch-blob upgrade, see #870 )

@tinovyatkin
Copy link
Member

BONUS: we don't skip any tests on Windows anymore

@tinovyatkin tinovyatkin mentioned this pull request Jun 10, 2020
3 tasks
@tinovyatkin
Copy link
Member

@xxczaki This is ready to merge 🎉

@tinovyatkin tinovyatkin requested a review from xxczaki June 10, 2020 20:25
package.json Outdated Show resolved Hide resolved
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.

None yet

10 participants