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

Strive for symmetrical API to browsers FormData. #124

Open
2 of 12 tasks
DylanPiercey opened this issue Jul 22, 2015 · 23 comments
Open
2 of 12 tasks

Strive for symmetrical API to browsers FormData. #124

DylanPiercey opened this issue Jul 22, 2015 · 23 comments
Assignees
Labels

Comments

@DylanPiercey
Copy link
Member

DylanPiercey commented Jul 22, 2015

It would be awesome if this library acted a bit more like a serverside polyfill for FormData in the browser.
I am very willing to create PR's for the items below. (Hopefully with help).

Here are a list of things that are currently missing (Feel free to comment more):

Existing Methods

  • FormData.append(String: field, String: value, Object: options)
  • Should allow for options to be a String that is the file name.
    https://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#dom-formdata-append
  • FormData.delete(String: field)
  • FormData.entries()
  • FormData.get(String: field)
  • FormData.getAll()
  • FormData.has()
  • FormData.keys()
  • FormData.set(String: field, String: value, Object: options)
  • FormData.values()

Other

  • Expose global FormData
  • Taken care of by isomorphic-form-data.
  • Support for blobs
  • Currently this is not supported - not sure if it's needed.
  • Polyfill in browser.js
  • Browser.js simply returns "FormData" currently. Perhaps we could consider adding a polyfill for the browser instead. Something like https://gist.github.com/Rob--W/8b5adedd84c0d36aba64.
    This may be out of scope.
@alexindigo
Copy link
Member

👍 Looks like a plan

@DylanPiercey
Copy link
Member Author

After realizing that the new methods are not in the spec and are specific to chrome and firefox I am unsure if we should implement them. However I think it would be good to leave this issue open for others to list api differences with the browser.

@DylanPiercey
Copy link
Member Author

@alexindigo What is your opinion on making form-data expose the global FormData?

This would likely be a 2.x thing, however I think it would position the library as more of a "browserfill", similar to node-fetch which exposes global fetch.

Unrelated:
You may find this interesting node-fetch/node-fetch#31.
Node-fetch will now automatically work with node-form-data :).

@alexindigo
Copy link
Member

Like global.FormData on library level?

And great job on bringing libraries closer together (but not too close to keep it PG) :) Thanks.

@DylanPiercey
Copy link
Member Author

global.FormData exactly. It's generally bad practice (obviously), however in the sense of a polyfill for FormData (on the server side) this would make sense and is exactly what node-fetch does.

To keep compatibility with older versions we could still expose FormData via module.exports but also expose it as a global.

@alexindigo
Copy link
Member

Main purpose of this module is to allow form submission from the server-side. Exposing global variables by default would be very unexpected by majority of consumers. But I see your desire for isomorphic all the things!™ :)

How about if we'd have another module, that would announce it's global nature more explicitly.

For example:

package.json:

{
  "name": "global-form-data",
  "dependencies":
  {
    "form-data": "*"
  }
}

index.js:

module.exports = global.FormData = require('form-data');

@DylanPiercey
Copy link
Member Author

@alexindigo I think that is a great idea since it wouldn't require a major release.
"global-form-data" would be nicely descriptive but part of me wants to suggest "node-form-data".
Either would do fine though.

The only concern is if users might find it confusing to have two packages - not sure if thats an issue.

@alexindigo
Copy link
Member

New module would behave exactly as the original one plus global export which is specified in the name.
And good readme file would always help.

Only confusion I can see could happen with versions, for that case owner of the new module, could stick to strict version dependency and update the package along with form-data.

Like:

{
  "name": "global-form-data",
  "version": "1.0.0-rc2",
  "dependencies":
  {
    "form-data": "1.0.0-rc2"
  }
}

@DylanPiercey
Copy link
Member Author

@alexindigo I think a * dependency would be fine in this case or at most a ^1.0 or 1.x.

As for names, after thinking about it a bit I quite like isomorphic-form-data. See https://github.com/matthew-andrews/isomorphic-fetch.

Either way I think this would be a great addition.

@DylanPiercey
Copy link
Member Author

@alexindigo What is your opinion on supporting passing a form element to the FormData constructor?

It may be a bit crazy - (Currently I have never needed this) - however after thinking about it, one potential use case would be supporting something like JS-DOM. This could make testing form submission with JSDOM a bit easier.

We could defer the actual form parsing logic to a dependency, keeping changes to this lib to a minimum.

Again this may not be particularly useful but I would like to know what others think.

@alexindigo
Copy link
Member

Btw, I found interesting thing:

Note: To be clear, the difference between FormData.set() and FormData.append() is that
if the specified key does already exist, FormData.set() will overwrite the existing value
with the new one, whereas FormData.append() will append the new value onto the end
of the existing values. See their dedicated pages for example code.

@alexindigo
Copy link
Member

As for form parsing, I think it should be different module as well. Maybe we'd expose more guts from FormData for other module to be able to use it in more efficient way.

Btw, we're creating new org for this project, so this new module and that isomorphic shim you mentioned could be under same umbrella if you like.

@simov
Copy link

simov commented Jul 26, 2015

Just a question: why is this module supposed to be a polyfill for the browser's FormData class? That scenario would be possible to use only via Browserify, Webpack or something like that, and then again I don't see the point.

I would rather like to see the actual multipart body generation cleaned up and extracted into a separate module along with all of the stream helper functions around it. If that layer is made good enough, then it will be much easier to extend it for other multipart types, while keeping the existing fixes.

From the request's perspective for example we don't even need a submit method.

@alexindigo
Copy link
Member

That's the plan, to make it library for libraries. We will split it. And yes Streams need more love. :)

First step would be test coverage, though.

@alexindigo
Copy link
Member

Thanks for the opinion btw.

@simov
Copy link

simov commented Jul 26, 2015

@alexindigo not a problem. I'm also not against the polyfill, I just want the generation logic as a separate module :)

Here are a few form-data related tests in request (in case they may help you):

@alexindigo
Copy link
Member

Yep, I do like leveldb approach. And thanks a lot for the tests. We'll incorporate them into new test suite.

@DylanPiercey
Copy link
Member Author

I think it would be awesome to have a library more focused on multipart generation without things like "submit". It would allow for us to more easily do a polyfill in the future.

Having said that this would involve turning the library into more of a utility with less features and with so many dependants I'm sure lots would break.

@alexindigo what are your thoughts on something like " form-data-core" (please better name) along side "form-data-polyfill" and "isomorphic-formdata"? 4 repos in total.

@alexindigo
Copy link
Member

@DylanPiercey I thought you'd like the idea of submit-less FormData :) Exactly like in the browser.

For now Grandiose Plan looks like following:

  • Cover everything with tests
  • Release 1.0 and start semvering
  • Start breaking things

And the actual libraries, we need something that does streaming multipart for sure.

Then bunch of plugins to determine content length and type.

And seems like we're about to have multiple aggregators like the one to be library for libraries, another one as browser polyfil and another one as server-side-only consumer friendly.

And if we can make them not to wrap one another, but assemble same components in different manner it will be uber awesome.

But it's my pipe dream, let's see how it goes. :)

@jimmywarting
Copy link

jimmywarting commented Oct 20, 2016

Also like the idea of submit-less formData

Working with github/fetch to support streaming... And this is how i took a sumbit-less native formdata to turn it to something readable: https://github.com/jimmywarting/fetch/blob/stream/fetch.js#L123

In chrome you can do this also: new Response(formdata).body.getReader() but I didn't have luxury since it was a polyfill

Just want to propose to you this polyfill https://github.com/jimmywarting/FormData/blob/master/FormData.js (it's built for the browser...) but you are free to copy as much code as you like from my polyfill if it's to any help

@andrewmclagan
Copy link

Any traction on this? A unified API would be amazing.

@tylerlong
Copy link

tylerlong commented May 11, 2020

As for form parsing, I think it should be different module as well. Maybe we'd expose more guts from FormData for other module to be able to use it in more efficient way.

Btw, we're creating new org for this project, so this new module and that isomorphic shim you mentioned could be under same umbrella if you like.

I there a way for parsing right now?

Update: you can always subclass FormData and change its behavior.

@mifi
Copy link

mifi commented Feb 16, 2022

https://github.com/octet-stream/form-data is a spec compliant alternative to this package

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

7 participants