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(Body): Added support for BodyMixin.formData() and constructing bodies with FormData #1314

Merged
merged 34 commits into from Nov 8, 2021

Conversation

jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Sep 26, 2021

Purpose

  • Add support for both response.formData() and request.formData()
  • Support constructing not only new Request(url, {body: formData}) but also new Response(formData) as it's supposed to be (only supported Request before...)
  • It's also able to convert a URLSearchParam to a formData as it dose in browser/spec

This would be a great addition so you can avoid having to depend on some external multiparty, formidable, busboy or any other body parser packages. that all have some different (non-speced) apis that you would need to learn.
Instead you could just do:

import { Response } from 'node-fetch'

http.createServer(async function (req, res) {
  const formData = await new Response(req, { headers: req.headers }).formData()
  const file = formData.get('avatar')
  
  var arrayBuffer = await file.arrayBuffer()
  var text = await file.text()
  var whatwgReadableStream = file.stream()
}) 

Changes

  • I changed the FormData test file to test more end to end instead.
  • Added a fast algoritm to parse formData

Additional information

Maybe thinking it would be nice to expose FormData, Blob and File from index.js so they are accessible, thoughts?
Think we have had this discussion before with exposing Blob before... #392


  • I updated ./docs/CHANGELOG.md
  • I updated readme
  • I added unit test(s)

closes #199
closes #1118
closes #1293
closes #876

@jimmywarting jimmywarting requested review from gr2m, LinusU and Richienb Sep 26, 2021
@jimmywarting jimmywarting changed the title Added support for body toFormData feat: Added support for body toFormData Sep 26, 2021
@jimmywarting jimmywarting changed the title feat: Added support for body toFormData feat(Body): Added support for body toFormData Sep 26, 2021
@jimmywarting jimmywarting changed the title feat(Body): Added support for body toFormData feat(Body): Added support for body.formData() Sep 26, 2021
@jimmywarting jimmywarting changed the title feat(Body): Added support for body.formData() feat(Body): Added support for body.formData() and constructing bodies with FormData Sep 26, 2021
@jimmywarting
Copy link
Collaborator Author

@jimmywarting jimmywarting commented Sep 26, 2021

Ready to be reviewed...

@jimmywarting jimmywarting mentioned this pull request Sep 26, 2021
4 tasks
package.json Outdated Show resolved Hide resolved
src/utils/multipart-parser.js Outdated Show resolved Hide resolved
src/body.js Show resolved Hide resolved
src/body.js Show resolved Hide resolved
src/utils/multipart-parser.js Outdated Show resolved Hide resolved
src/utils/multipart-parser.js Outdated Show resolved Hide resolved
src/utils/multipart-parser.js Outdated Show resolved Hide resolved
src/utils/multipart-parser.js Outdated Show resolved Hide resolved
src/utils/multipart-parser.js Outdated Show resolved Hide resolved
src/utils/multipart-parser.js Outdated Show resolved Hide resolved
LinusU
LinusU approved these changes Oct 15, 2021
Copy link
Member

@LinusU LinusU left a comment

Sorry for the dragged out review, I've been swamped lately...

Personally, there are a few things that I would like to clean up more, but I could send those in a follow up PR if you would prefer that? As I understood it this code was copied from another package, do you have a link to that package? (e.g. I think that the mark/clear functions are quite hard to follow, and could be done in a better way)

The only thing that has me a bit weary right now is the return statements inside the for loop. I really think that this parsing will return garbage data back if the response is malformed. Personally, I think it's better to throw in those cases, so that no one starts relying on improper parsing which we might break in a future update. That being said, since you did say that you wanted to leave it as is I won't push it further 😅

Would you be open to me addressing that in a follow up PR, after testing what the browser does, or do you want that behaviour?

Apart from that, great PR! Nice to get this in 👍


Also, I'm also maintaining Multer which parses multipart data, and I've been interested in switching out the parser to a bit more modern/simpler one. What do you think about keeping this code in a separate package that both node-fetch and multer could depend on?

@gr2m
Copy link
Collaborator

@gr2m gr2m commented Oct 15, 2021

What do you think about keeping this code in a separate package that both node-fetch and multer could depend on?

I think that'd be great, as long as it doesn't conflict with the standard compatibility of fetch

@jimmywarting
Copy link
Collaborator Author

@jimmywarting jimmywarting commented Oct 15, 2021

Sorry for the dragged out review, I've been swamped lately...

Personally, there are a few things that I would like to clean up more, but I could send those in a follow up PR if you would prefer that? As I understood it this code was copied from another package, do you have a link to that package? (e.g. I think that the mark/clear functions are quite hard to follow, and could be done in a better way)

The code is from an older version of formidable that i ported to https://github.com/github/fetch a while back (to be more browser friendlier (without the need of having node:streams but got rejected due to the fact that there was no streaming method in xhr)

Here is theirs new and updated code: https://github.com/node-formidable/formidable/blob/master/src/parsers/Multipart.js

Formidable have been tested with a variety of different multipart uploads and it seems pretty speedy at parsing the payload.
If we keep changing it then i think we should be bringing in theirs test cases as well... I do not know how spec compatible it is.

This is the official spec: https://andreubotella.com/multipart-form-data

The only thing that has me a bit weary right now is the return statements inside the for loop. I really think that this parsing will return garbage data back if the response is malformed. Personally, I think it's better to throw in those cases, so that no one starts relying on improper parsing which we might break in a future update. That being said, since you did say that you wanted to leave it as is I won't push it further 😅

Would you be open to me addressing that in a follow up PR, after testing what the browser does, or do you want that behaviour?

Sure, As long as the body can be splittet up in chunks (anywhere) and parse different amount of bytes (even if it's splited in the middle of a boundary) and be streamable, then i'm fine with it.

It would also be nice if we could write very large files to the disk instead of keeping it in the memory also. and then use our blobs blobFrom(path, [mimetype]) later on.


Also, I'm also maintaining Multer which parses multipart data, and I've been interested in switching out the parser to a bit more modern/simpler one. What do you think about keeping this code in a separate package that both node-fetch and multer could depend on?

I think this would be a grate fit for the stream-consumers it's mostly a straight out copy from undici BodyMixin that made it into node.js core. it already uses fetch-blob and is also written in ESM + that it's new so not so many breaking changes for some ppl
It's something i have been wanting to bring into node-fetch anyway: #1252

It dose not have formData doe since FormData isn't implemented in NodeJs but it could be one day? there is issues about bringing in FormData into node
Also saw that @octet-stream linked undici to this PR: nodejs/undici#974

Personally i would like to remove the .buffer() method in my stream consumer package also since i think it should be discouraged to use it anywhere outside of NodeJS. you can just as easily write arrayBuffer(iterable).then(Buffer.from) if you really need a node:buffer

Richienb
Richienb previously requested changes Oct 16, 2021
docs/CHANGELOG.md Outdated Show resolved Hide resolved
src/body.js Outdated Show resolved Hide resolved
@LinusU
Copy link
Member

@LinusU LinusU commented Oct 16, 2021

Formidable have been tested with a variety of different multipart uploads and it seems pretty speedy at parsing the payload. If we keep changing it then i think we should be bringing in theirs test cases as well... I do not know how spec compatible it is.

I think that we are already working in a different way in the specific return statements that I talked about, because their write methods returns the number of bytes consumed, whereas our version does. So that's why I'm wary about that specific case.

Porting over the tests would be great, also adding some test case that we run both against our parser and the browser equivalent...

Also, I'm also maintaining Multer which parses multipart data, and I've been interested in switching out the parser to a bit more modern/simpler one. What do you think about keeping this code in a separate package that both node-fetch and multer could depend on?

I think this would be a grate fit for the stream-consumers it's mostly a straight out copy from undici BodyMixin that made it into node.js core. it already uses fetch-blob and is also written in ESM + that it's new so not so many breaking changes for some ppl It's something i have been wanting to bring into node-fetch anyway: #1252

It dose not have formData doe since FormData isn't implemented in NodeJs but it could be one day? there is issues about bringing in FormData into node Also saw that @octet-stream linked undici to this PR: nodejs/undici#974

For Multer I would probably need something a bit more low level, since we are doing field and size validation on the fly, and also stream the files to temporary files on disk 🤔

Depends on how the API would look of course!

Personally i would like to remove the .buffer() method in my stream consumer package also since i think it should be discouraged to use it anywhere outside of NodeJS. you can just as easily write arrayBuffer(iterable).then(Buffer.from) if you really need a node:buffer

👍

@octet-stream
Copy link
Contributor

@octet-stream octet-stream commented Oct 16, 2021

I was kind of thinking to make a low-level multipart parser as part of my form-data-encoder that could've basically get an AsyncIterable<Uint8Array> as the input and give API to attach callback to get parts once they parsed. I didn't come up with the look of that API yet, and not yet sure if I could make thing like that (never done any complex parser before), but it still may be implemented there at some point in a future.

@jimmywarting
Copy link
Collaborator Author

@jimmywarting jimmywarting commented Oct 16, 2021

Personally i would like to remove the .buffer() method in my stream consumer package also since i think it should be discouraged to use it anywhere outside of NodeJS. you can just as easily write arrayBuffer(iterable).then(Buffer.from) if you really need a node:buffer

👍

fyi, I already went ahead and removed it... (put a warning sign on it)

# Conflicts:
#	package.json
#	src/utils/form-data.js
@LinusU
Copy link
Member

@LinusU LinusU commented Nov 5, 2021

@jimmywarting seems to be a conflict in package.json, could you fix that?

@Richienb would you be able to review this? ☺️

@jacob-ebey
Copy link

@jacob-ebey jacob-ebey commented Nov 6, 2021

Oh this is exciting. Can't wait to see this merged.

@jimmywarting jimmywarting merged commit 3944f24 into main Nov 8, 2021
9 checks passed
@jimmywarting jimmywarting deleted the feature/body-formData branch Nov 8, 2021
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.

6 participants