melange-community / melange-fetch

Fetch bindings for Melange
MIT License
203 stars 53 forks source link

Bind form data #35

Closed anmonteiro closed 4 years ago

anmonteiro commented 4 years ago

This is a rebase of #14 on current master, done after obtaining permission from @fakenickels.

Question: what's the point of committing the lib/ directory? It only contains JS files generated by the compiler, and really pollutes the diffs.

yawaramin commented 4 years ago

Huh, that's funny. The main reason I thought FormData bindings should go in Webapi is because of the new FormData(HTMLFormElement) constructor. But maybe there's another way. How about making bs-webapi depend on bs-fetch? That way we don't have yet another package to maintain. And semantically it makes sense; the 'Web APIs' can be considered to contain the 'Fetch API'.

I can create a binding Webapi.Dom.HtmlFormElement.data : t -> FormData.t which calls the constructor, and we can put a note in the FormData module to see that binding if you're looking for that constructor.

EDIT: to clarify, I'm suggesting to put the FormData/Blob/File/Iterator bindings in bs-fetch, rather than in bs-webapi. Although tbh the Iterator bindings should really go in BuckleScript's Js module...

anmonteiro commented 4 years ago

I like that idea. @yawaramin feel free to push to this PR if you see fit, or otherwise open a pre-requisite PR that I can rebase over.

glennsl commented 4 years ago

If it was only FormData I think that would be fine. FormData is technically XHR, but close enough! Blob and File are much more general-purpose though.

From https://developer.mozilla.org/en-US/docs/Web/API/File:

File objects are generally retrieved from a FileList object returned as a result of a user selecting files using the element, from a drag and drop operation's DataTransfer object, or from the mozGetAsFile() API on an HTMLCanvasElement.

A File object is a specific kind of a Blob, and can be used in any context that a Blob can. In particular, FileReader, URL.createObjectURL(), createImageBitmap(), and XMLHttpRequest.send() accept both Blobs and Files.

So I don't think this is going to work very well long-term, and it's going to make migration later really awkward.

yawaramin commented 4 years ago

Hmm, I was afraid of that. So the best scenario is to have Blob/File/Iterator/Stream in BuckleScript's Js module? I think we could still keep them in bs-fetch for the interim and then gracefully alias them once they land in BuckleScript and both bs-fetch and bs-webapi upgrade to the latest bs-platform?

anmonteiro commented 4 years ago

my proposal is we keep the types for those Blob / File, etc in bs-fetch and have bs-web-api depend on this library.

In this case, bs-fetch only owns the types, and bs-web-api can implement the functionality.

Is that aligned with your thinking?

yawaramin commented 4 years ago

I think though that a bs-fetch user would want to be able to do some operations on at least blobs. E.g. ( https://developer.mozilla.org/en-US/docs/Web/API/Body/blob ):

response.blob().then(function(myBlob) {
  // do something with myBlob
});

If we tell the user at this point 'to do anything with the blob you need bs-webapi', that's frustrating. Would be better to have the operations handy in bs-fetch itself.

anmonteiro commented 4 years ago

I think that's already the case in this library, and IMO it would be out of scope for this change.

glennsl commented 4 years ago

So the best scenario is to have Blob/File/Iterator/Stream in BuckleScript's Js module?

I would have it in a separate package to avoid depending on bs-platform's release schedule and erratic versioning. I'd even suggest moving the Dom types out of bs-platform to this package.

But as long as the types are aliased properly, I don't think it should cause any long-term problems to keep them in bs-fetch. I'm more worried about having the Blob and File bindings there. The impact would be reduced if they're aliased in bs-webapi as well, but there's still going to be people using those directly and so it's going to cause some pain to move them later.

You're the ones actually using this though, so I'll go along with whatever you agree on.

yawaramin commented 4 years ago

OK let's keep blob and file here in bs-fetch, because we'll need them to do the bindings properly. We'll also need to have Iterator here because FormData needs that as well. With the intention of aliasing it to Js.Iterator if/when that lands in bs-platform.

I will flesh out the implementations of blob and file in bs-webapi, adding bs-fetch as a dependency there.

This is purely out of laziness on my part, I don't want to end up with another library to extract common parts of bs-fetch and bs-webapi 😁

yawaramin commented 4 years ago

@glennsl thanks for the review, will address after work.