Bind form data #35

anmonteiro · 2020-07-13T12:44:52Z

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.

anmonteiro · 2020-07-13T12:46:26Z

src/Fetch.ml

@@ -379,6 +379,29 @@ module Response = struct
  external clone : t = "clone" [@@bs.send.pipe: t]
 end

+(* This belongs to XHR spec and will probably be moved to another repo in the future *)
+
+module Blob = struct


wonder if we need this Blob module given that there's a type blob above in this file.

anmonteiro · 2020-07-13T12:50:17Z

src/Fetch.ml

+  external make : unit -> t = "FormData" [@@bs.new]
+
+  (* Added for React Native polyfill compatibility *)
+  external appendObject : string -> < .. > Js.t -> unit = "append" [@@bs.send.pipe : t]


I think this needs to be restricted to a specific shape, given this comment: #14 (comment)

Yeah, it looks like a specific shape, but I have no idea really. I know very little about React Native.

glennsl

Seems like a funny coincidence that @yawaramin just submitted reasonml-community/bs-webapi-incubator#187 as well. I think we ought to avoid duplicating bindings and especially types, but also want to avoid bs-fetch depending on bs-webapi. Perhaps creating a new bs-webapi-types project might be an idea at least? What do you think @yawaramin and @anmonteiro?

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

It was useful early on when minor releases of BuckleScript randomly broke things, and when this was updated more frequently. Nowadays it's less useful, but can still be nice to review the code generation for PRs. I've caught several cases of unfortunate code generation this way, although I would probably have caught them anyway as I'm familiar enough with the code generation to know what'll produce good and bad code. Much less useful when there's as much noise as this, but I don't mind it much. PRs here are usually not very big anyway.

yawaramin · 2020-07-13T19:02:52Z

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 · 2020-07-13T19:15:04Z

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 · 2020-07-13T19:15:29Z

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 · 2020-07-13T19:25:09Z

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 · 2020-07-13T19:56:55Z

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 · 2020-07-13T20:09:13Z

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 · 2020-07-13T20:26:58Z

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

glennsl · 2020-07-13T21:39:26Z

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 · 2020-07-13T22:11:22Z

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 😁

As per PR discussion, will put more fleshed-out Blob and File bindings in bs-webapi, and keep FormData bindings in bs-fetch.

If the iterator did not produce a value this time but is not finished, keep iterating.

glennsl · 2020-07-14T09:59:05Z

src/Fetch.ml

+    let classify t =
+      if Js.typeof t = "string" then `String (Obj.magic t)
+      else `File (Obj.magic t)


Obj.magic should have both its input and output types annotated to contain it as much as possible. If this was used internally it would be completely unsound since mlis don't apply constraints internally.

Suggested change

let classify t =

if Js.typeof t = "string" then `String (Obj.magic t)

else `File (Obj.magic t)

let classify (t: t) =

if Js.typeof t = "string" then

`String (Obj.magic t: string)

else

`File (Obj.magic t: file)

glennsl · 2020-07-14T10:07:26Z

src/Fetch.ml

+  external appendObject : name:string -> value:< .. > Js.t -> ?filename:string -> unit =
+    "append" [@@bs.send.pipe : t]
+
+  external appendBlob : name:string -> value:blob -> ?filename:string -> unit =
+    "append" [@@bs.send.pipe : t]
+
+  external appendFile : name:string -> value:file -> ?filename:string -> unit =
+    "append" [@@bs.send.pipe : t]
+
+  external setObject : name:string -> value:< .. > Js.t -> ?filename:string -> unit =
+    "set" [@@bs.send.pipe : t]
+
+  external setBlob : name:string -> value:blob -> ?filename:string -> unit =
+    "set" [@@bs.send.pipe : t]
+
+  external setFile : name:string -> value:file -> ?filename:string -> unit =
+    "set" [@@bs.send.pipe : t]


These labels seem unnecessary to me. First because it seems unlikely that anyone's going to confuse the order of name and value, as this is pretty obvious and consistent over any API I've ever seen in any language. Secondly, since this binds to an existing API, there's both precedent for and an expectation for it to be unlabelled to model it as closely as possible.

yawaramin · 2020-07-14T18:08:48Z

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

- Annotate `classify` return type to prevent unsoundness - Remove `name` and `value` labels

glennsl · 2020-07-15T12:08:39Z

src/Fetch.ml

+    let classify t : [> `String of string | `File of file] =
+      if Js.typeof t = "string" then `String (Obj.magic t)
+      else `File (Obj.magic t)


This still doesn't constrain the input argument, so you could for example do classify 42 which would return an invalid `File value back.

Suggested change

let classify t : [> `String of string | `File of file] =

if Js.typeof t = "string" then `String (Obj.magic t)

else `File (Obj.magic t)

let classify (t: t) : [> `String of string | `File of file] =

if Js.typeof t = "string" then `String (Obj.magic t)

else `File (Obj.magic t)

Will change that.

glennsl · 2020-07-15T12:12:28Z

src/Fetch.ml

+  external keys : t -> string Iterator.t = "keys" [@@bs.send]
+  external values : t -> EntryValue.t Iterator.t = "values" [@@bs.send]
+
+  external appendObject : string -> _ Js.t -> ?filename:string -> unit =


This is a bit nitpicky, but <..> Js.t is more accurate than _ Js.t. With this you could pass in an int Js.t for examplle, which would of course be silly, but what if at some point in the future BuckleScript decides to use Js.t to distinguish JS and OCaml semantics of something other than objects?

I don't think there will ever be any way to create a valid int Js.t :-) BuckleScript libs written by Hongbo are also using something similar: https://bucklescript.github.io/bucklescript/api/Js.Obj.html#VALkeys (val keys : 'a Js.t -> string array)

Most of the functions there use < .. > Js.t, and Bob's not exactly known for his consistency. But mostly I just don't see any good reason to be deliberately vague. It's not a technical improvement, there could be issues with it in the future, and being vague doesn't generally make for better documentation.

Fair enough, I'll change it :-)

glennsl

Looks good. Thanks both!

fakenickels added 7 commits July 13, 2020 05:35

Upgrade bs-platform

eba193e

Add FormData bindings

877c5da

Add FormData demo

1221cc7

Prefix methods with mutable

1188b1f

Ignore yarn-error.log

3b23f1b

Remove mutable prefixes and add setBlob

f1f24ba

Fix OCaml demo

7c00c51

anmonteiro commented Jul 13, 2020

View reviewed changes

glennsl reviewed Jul 13, 2020

View reviewed changes

yawaramin added 4 commits July 13, 2020 20:24

Complete FormData bindings

97da43b

As per PR discussion, will put more fleshed-out Blob and File bindings in bs-webapi, and keep FormData bindings in bs-fetch.

Fix extra currying

6fca44b

Fix setFile type

3da864e

Adjust iteration logic

1bbdc34

If the iterator did not produce a value this time but is not finished, keep iterating.

glennsl reviewed Jul 14, 2020

View reviewed changes

Address PR comments

9401c28

- Annotate `classify` return type to prevent unsoundness - Remove `name` and `value` labels

yawaramin requested a review from glennsl July 14, 2020 22:23

Fix ocamldoc link

68814b8

glennsl reviewed Jul 15, 2020

View reviewed changes

Make requested changes

1485510

glennsl approved these changes Jul 16, 2020

View reviewed changes

glennsl merged commit 90aa341 into master Jul 16, 2020

glennsl deleted the anmonteiro/bind-form-data branch July 16, 2020 09:48

yawaramin mentioned this pull request Jul 16, 2020

Feature/bind form data #14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bind form data #35

Bind form data #35

anmonteiro commented Jul 13, 2020 •

edited

anmonteiro Jul 13, 2020

anmonteiro Jul 13, 2020

glennsl Jul 13, 2020

glennsl left a comment

yawaramin commented Jul 13, 2020 •

edited

anmonteiro commented Jul 13, 2020

glennsl commented Jul 13, 2020 •

edited

yawaramin commented Jul 13, 2020

anmonteiro commented Jul 13, 2020

yawaramin commented Jul 13, 2020

anmonteiro commented Jul 13, 2020

glennsl commented Jul 13, 2020 •

edited

yawaramin commented Jul 13, 2020

glennsl Jul 14, 2020

glennsl Jul 14, 2020

yawaramin commented Jul 14, 2020

glennsl Jul 15, 2020

yawaramin Jul 15, 2020

glennsl Jul 15, 2020

yawaramin Jul 15, 2020

glennsl Jul 15, 2020

yawaramin Jul 15, 2020

glennsl left a comment

Bind form data #35

Bind form data #35

Conversation

anmonteiro commented Jul 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsl left a comment

Choose a reason for hiding this comment

yawaramin commented Jul 13, 2020 • edited

anmonteiro commented Jul 13, 2020

glennsl commented Jul 13, 2020 • edited

yawaramin commented Jul 13, 2020

anmonteiro commented Jul 13, 2020

yawaramin commented Jul 13, 2020

anmonteiro commented Jul 13, 2020

glennsl commented Jul 13, 2020 • edited

yawaramin commented Jul 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yawaramin commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsl left a comment

Choose a reason for hiding this comment

anmonteiro commented Jul 13, 2020 •

edited

yawaramin commented Jul 13, 2020 •

edited

glennsl commented Jul 13, 2020 •

edited

glennsl commented Jul 13, 2020 •

edited