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

Adding HTML form data handling to core #38943

Closed
StarpTech opened this issue Jun 5, 2021 · 21 comments
Closed

Adding HTML form data handling to core #38943

StarpTech opened this issue Jun 5, 2021 · 21 comments
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. stale

Comments

@StarpTech
Copy link

StarpTech commented Jun 5, 2021

Is your feature request related to a problem? Please describe.

Provide a convenient way to parse and create forms according to https://datatracker.ietf.org/doc/html/rfc7578 and https://xhr.spec.whatwg.org/#interface-formdata

Describe the solution you'd like
Implementation examples:

Describe alternatives you've considered

Form constructing

Form parsing

Adding new features to the core is a controversial topic. The procedure in #19308 (comment) might help.

Is it hard to be done right?

Yes in a performant way with good API design.

Is it widely used?

Yes, it is.

Does it have a clear spec?

Yes.

Is it likely to become obsolete in the near future?

No.

Does it need optimizations from the core?

Not necessarily.

Will the entire community benefit from it being part of the core?

Yes, it's essential to deal with forms in almost any app.

@Trott
Copy link
Member

Trott commented Jun 6, 2021

@bmeck You proposed adding mime to core at some point, didn't you?

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jun 6, 2021
@mcollina
Copy link
Member

mcollina commented Jun 6, 2021

I think the key question to answer is what would add to have this in core. Maintaining things in core is harder than maintaining them in the ecosystem and it adds a burden to a limited set of folks.

@StarpTech
Copy link
Author

StarpTech commented Jun 6, 2021

I think the key question to answer is what would add to have this in core. Maintaining things in core is harder than maintaining them in the ecosystem and it adds a burden to a limited set of folks.

MIME is as stable and important as HTML itself. The implementation should only provide minimal functionality like parsing and stream handling. This allows the community to add additional functionality on top of it. Currently, this is reimplemented every time. I think the maintenance burden is very low until the module is stabilized.

@bmeck
Copy link
Member

bmeck commented Jun 6, 2021 via email

@StarpTech
Copy link
Author

StarpTech commented Jun 6, 2021

Do you mean #21128?

Maybe I miss something but I don't propose mime type handling but MIME multipart parsing.

@bl-ue
Copy link
Contributor

bl-ue commented Jun 6, 2021

(off-topic: that PR is the oldest one open :)

@MylesBorins
Copy link
Member

MylesBorins commented Jun 10, 2021

I'm +1 on continuing to try and get a MIME module into the runtime based on WHATWG spec, that said I think that is a little bit lower level than a full multipart form constructor / parser.

I'm not 100% that we should support the a Form constructor / parser ourselves tbh... although if we were going to explore this I feel like starting in unidici (the next generation http client) might be a good place to focus @nodejs/undici thoughts?

edit: just adding context... I'm not convinced right now that form creation / parsing is a consistent level of abstraction in our platform. We don't offer parsing / creation for other standard data formats (e.g. XML) and while it would be great to have a single implementation for our ecosystem to use, I don't know that the maintenance cost is worth it at the moment. If we do implement fetch it might be worth implementing the FormData constructor from the WHAT WG spec, although we should explore if this can be handled well in userland.

undici-fetch doesn't implement form-data, that might be a good place to explore implementing the spec and figuring out what parts are missing from core to make this easy such as Blob and Mime

@StarpTech
Copy link
Author

StarpTech commented Jun 10, 2021

I'm not convinced right now that form creation / parsing is a consistent level of abstraction in our platform.

Node.js positioned itself as "Node.js designed to build scalable network applications". Sending files from the client to the server is daily business. To explain to a new user that this can't be done easily with Node.js is crazy. I experienced a very bad experience by writing a new abstraction for https://github.com/fastify/fastify-multipart.

I think it can be very valuable to provide a thin module that is only responsible to parse forms. Similar to https://golang.org/pkg/mime/multipart/#example_NewReader

If we can't benefit from the core, it would be also fine to host a module under the Node.js organization as with undici and reference it in the docs.

I think the idea to extend the standard library with regular npm modules very attractive. This is the same strategy we are doing with fastify. House current, high-quality core modules to the community.

Btw: For example, all rust standard modules are crate packages. This can simplifying contribution too.

@Trott
Copy link
Member

Trott commented Jun 23, 2021

We talked about this briefly in the TSC meeting today, but didn't get too far without @mcollina and @MylesBorins. However, it seems that it ties into the larger issue of how to decide what goes in core at nodejs/TSC#1041 which is also being discussed by the TSC.

@jasnell
Copy link
Member

jasnell commented Jun 23, 2021

FWIW, I have implementation of FormData in my todo list for this year as part of the fetch API implementation effort.

(in case the connection is not clear... fetch uses the Body mixin, which includes a method for reading the body of a response as FormData)

@mmarchini mmarchini removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jul 1, 2021
@mmarchini
Copy link
Contributor

Removed agenda label until nodejs/TSC#1041 is resolved

@jimmywarting
Copy link

jimmywarting commented Aug 5, 2021

Implementing FormData requires that we add the File class (#39015) first.
Blob and Files is part of the requirements needed to build a proper spec compatible FormData

Append/Set a Blob entry requires you to upgrade the Blob to a File instance so the FormData can be serialized properly later.

I definitely think we should add it to core as well. It can be shipped before fetch

@jimmywarting
Copy link

jimmywarting commented Aug 5, 2021

fyi, there are two other spec compatible FormData in the userland/npm

  • My own formdata-polyfill (quite minimal, no extra glyphs or extra features - just the right amount).
    My tests cases runs the same WPT test for both the FormData Implementation and the serializer (aka encoder, aka formDataToBlob). You have my permission to copy the hole codebase and use whatever license you want.
  • formdata-node (uses typescript and some of the stuff from form-data implemented like getComputedLength, boundary stream Symbol.asyncIterator properties added to the FormData instance that isn't part of the spec, he is attempting to remove it in the upcoming 4th version and replace it with form-data-encoder - I'm not sure of how spec compatible it is but it gets the job done )

node-fetch plans to remove support for form-data

@targos targos added feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. labels Aug 9, 2021
@jimmywarting
Copy link

jimmywarting commented Nov 9, 2021

node-fetch@3 just got support for decoding FormData and URLSearchParams payloads back to a FormData instance 🎉

fd = await new Response(new FormData()).formData()
fd = await new Response(new URLSearchParams()).formData()

Now you don't really need formidable, busboy or anything like it to decode multipart payloads, you can just do something like this instead:

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')
  
  const arrayBuffer = await file.arrayBuffer()
  const text = await file.text()
  const whatwgReadableStream = file.stream()
}) 

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label May 9, 2022
@koresar
Copy link

koresar commented May 10, 2022

Can we conclude that FormData is now supported by the node built-in fetch?
Or not yet?

@mcollina
Copy link
Member

FormData is not a great abstractions for parsing forms. It's not a streaming implementation and therefore it is hardly an API I would use at scale.

@github-actions github-actions bot removed the stale label May 11, 2022
@jimmywarting
Copy link

jimmywarting commented May 11, 2022

It's not a streaming implementation

You are right about that. I would maybe not use it either for large uploads as it's right now (but it's possible to change that). but for smaller payloads where file isn't uploaded then i would use it for thing such as url encoded payload for small simple forms.

There is also one thing in the blob behavior that i have been meaning to add into node-fetch and fetch-blob in that way it can pag data to the file system when it holds too much data. So that when you parse a payload with request.formdata() then the files would streamed & written to a place on the harddrive and the File instance you get back from eg: const file = fd.get('uploads') would just be a reference point to somewhere on the hard drive. meaning that it wouldn't allocate any memory.

If the in-memory space for blobs is getting full, or a new blob is too large to be in-memory, then the blob system uses the disk. This can either be paging old blobs to disk, or saving the new too-large blob straight to disk.

https://chromium.googlesource.com/chromium/src/+/HEAD/storage/browser/blob/README.md

It's a bit surprising that neither Deno and NodeJS haven't understood this and implemented it this way. b/c right now they are pretty useless in the way that Blobs can only be constructed only using allocated data from the memory and not be refered to some place on the hard drive or even getting a new File instance backed up bt the hard drive.

If new large files parsed from request.formData() is written straight to a temp folder then i would for sure use formData parsing also for all my requests. browser can do some pretty smart tricks to blobs that make it so that when you read, slice or move the data. then it never touches the javascript thread


fetch-blob is the only blob impl that got some stuff right by implementing some blobFrom(path, [mimetype]) helper methods that dosen't allocate any data into the memory. And it's the only possible solution you have for creating new FormData instances where you need to upload large amount of data using multipart upload + FormData.

what NodeJS needs is #37340 to make this somewhat feasible/usable

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Nov 8, 2022
@mcollina
Copy link
Member

mcollina commented Nov 8, 2022

FormData is now part of Node.js core as a fetch / undici.
Fetch allows to send and receive multipart data.

@mcollina mcollina closed this as completed Nov 8, 2022
@qubyte
Copy link
Contributor

qubyte commented Apr 7, 2024

Forgive me for digging up an old issue, but the history is relevant. While form creation is in Node now via FormData, form parsing (as requested in the issue description) is not. Can this issue be reopened, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem. stale
Projects
Development

No branches or pull requests