-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add FormData under melange.dom #1153
Conversation
jscomp/others/dom_formData.ml
Outdated
let classify : entryValue -> [> `String of string | `File of file ] = | ||
fun t -> | ||
if Js.typeof t = "string" then `String (Obj.magic t) | ||
else `File (Obj.magic t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really happy with the classify, but I haven't seen any other way to achieve support for File
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show me examples of why classify
is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reading it in reasonml/reason-react#846, but I wonder whether this is needed at all or if folks can classify it downstream, or even use Js.Types.classify
.
Besides, if we were to keep this, I'd rather call it Blob
than File
, since File
is a subclass of Blob
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally, the discussion for 0 runtime goes away if we remove this. If it's needed can be added in webapi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a bit weird that entryValue is provided by this module but unnaccessible from this module, but it's simpler to keep it like this
How is this working in DOM? my understanding is that this code is being added to Lines 8 to 20 in dd13b62
this code doesn't compile locally:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue with form data here is that it sort of breaks the assumption that melange.dom
only has types, and no functions.
What do you think about adding the necessary types in this PR and then making a small library for form data using those types?
jscomp/others/dom_formData.ml
Outdated
type t | ||
|
||
external make : unit -> t = "FormData" [@@mel.new] | ||
external append : string -> string -> unit = "append" [@@mel.send.pipe: t] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
append
also takes a ?filename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
append can't have a filename when it's a string. appendBlob
or appendFile
is used and have the ?filename there
jscomp/others/dom_formData.ml
Outdated
external make : unit -> t = "FormData" [@@mel.new] | ||
external append : string -> string -> unit = "append" [@@mel.send.pipe: t] | ||
external delete : string -> unit = "delete" [@@mel.send.pipe: t] | ||
external get : string -> entryValue option = "get" [@@mel.send.pipe: t] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function doesn't return entryValue option
, but rather null
when not found. we should use [@@mel.return null_to_opt]
jscomp/others/dom_formData.ml
Outdated
external delete : string -> unit = "delete" [@@mel.send.pipe: t] | ||
external get : string -> entryValue option = "get" [@@mel.send.pipe: t] | ||
external getAll : string -> entryValue array = "getAll" [@@mel.send.pipe: t] | ||
external set : string -> string -> unit = "set" [@@mel.send.pipe: t] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set also takes ?filename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as append
. Can't have a filename when it's a string. setObject or setFile or setBlob can be used and have ?filename
Do you prefer to keep bindings outside of melange.dom? There's currently dom_storage (and this PR adds |
As far as I know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that FormData
isn't really specific to JS environments that have a DOM, so this could rather be added to Js.FormData
. what do you think?
ec98989
to
0c78f80
Compare
That make sense, moved also https://github.com/melange-re/melange/pull/1218/files into js |
Don't understand why opam macos fails while ubuntu is ok |
0c78f80
to
194c246
Compare
@davesnx what do you think of the new API for |
in 32cf627 I changed it again to take the distinction between blobs / primitives into account (the functions only accept filename as a third argument if the 2nd arg is a |
Co-authored-by: David Sancho <[email protected]>
f1d9347
to
8f9f23b
Compare
Moved and adapted FormData from https://github.com/melange-community/melange-fetch/blob/master/src/Fetch.ml
Since reasonml/reason-react#846, React would benefit to have FormData as part of melange.dom rather than their own bindings
PS. Fixes a documentation typo in the first commit.