feat: add js_{weakset,weakmap} functions #1058
Conversation
jscomp/runtime/js_set.ml
Outdated
| external make : unit -> 'a t = "Set" [@@mel.new] | ||
| external fromArray : 'a array -> 'a t = "Set" [@@mel.new] | ||
|
|
||
| (* commented out until Melange has a plan for iterators |
There was a problem hiding this comment.
We could copy this module from rescript-core: https://github.com/rescript-association/rescript-core/blob/main/src/Core__Iterator.res
WDYT?
There was a problem hiding this comment.
I think it's not a bad idea. Looks minimal enough to do what we'd want at first.
|
Oops, I just noticed that there's already a PR for Js.set #1047 (comment) 🙈 |
There was a problem hiding this comment.
this is great, thanks so much!
Next steps here are:
- Move your improvements to Js.Set to feat: add bindings to
Js.Set#1047 - open a new PR with the proposed iterator stuff
- merge this one with only the new bindings / tests
jscomp/runtime/js_set.ml
Outdated
| @@ -1,3 +1,20 @@ | |||
| (** ES6 Set API *) | |||
|
|
|||
| type 'a t | |||
|
|
|||
| external make : unit -> 'a t = "Set" [@@mel.new] | |||
| external fromArray : 'a array -> 'a t = "Set" [@@mel.new] | |||
There was a problem hiding this comment.
I like fromArray, which I haven't included in #1047. would you like to make that change to my PR?
jscomp/runtime/js_set.ml
Outdated
| external make : unit -> 'a t = "Set" [@@mel.new] | ||
| external fromArray : 'a array -> 'a t = "Set" [@@mel.new] | ||
|
|
||
| (* commented out until Melange has a plan for iterators |
There was a problem hiding this comment.
I think it's not a bad idea. Looks minimal enough to do what we'd want at first.
tatchi
changed the title
jscomp/runtime/js_weakset.ml
Outdated
| @@ -1,3 +1,8 @@ | |||
| (** ES6 WeakSet API *) | |||
|
|
|||
| type 'a t | |||
|
|
|||
| external make : unit -> 'a t = "WeakSet" [@@mel.new] | |||
| external add : value:'a -> 'a t = "add" [@@mel.send.pipe: 'a t] | |||
There was a problem hiding this comment.
btw this is not 100% safe becausevalue must be an object. Trying to add a primitive value like an int fails.
There was a problem hiding this comment.
so should we make it take a _ Js.t or a _ Js.Dict.t instead?
There was a problem hiding this comment.
I don't know. If I'm not mistaken, this means it would not be possible to pass normal OCaml record while I guess it should be ok since they compile to JS objects. See example 1097608 (#1058)
There was a problem hiding this comment.
that's fair, but at least with _ Js.t we can know that it's exactly a JS object.
To me, the tradeoff here seems like:
- if you keep
'a, you can pass any argument to the functions in the module, at the cost of runtime errors when you pass non-object arguments - if you enforce
_ Js.t, then you lose the ability to pass some well-known-to-work types (such as records, which are nominal, and you can't make a function accept "all records")
Personally, I'd prefer if we shipped 99% runtime-free, correct-by-construction code. IMO it's also pretty easy to change it downstream:
includetheJs.WeakMapmodule and overwrite the externals you wish to change signatures- use
external toJsDotT: myRecord -> _ Js.t = "%identity"to convert your records / other types that compile to objects at the boundary so that you can pass them toJs.WeakMap.
What do you think?
There was a problem hiding this comment.
I couldn't use Js.t because the Js module is not accessible from jscomp/runtime and I couldn't find an alternative in Js_*. So I ended up using Js_dict.t.
There was a problem hiding this comment.
I think that's a fair compromise.
jscomp/runtime/js_weakmap.ml
Outdated
| external get : key:'k -> 'v option = "get" [@@mel.send.pipe: ('k, 'v) t] | ||
| external has : key:'k -> bool = "has" [@@mel.send.pipe: ('k, 'v) t] | ||
|
|
||
| external set : key:'k -> value:'v -> ('k, 'v) t = "set" |
There was a problem hiding this comment.
Same here, key must be an object
anmonteiro
force-pushed
the
set-bindings
branch
from
621aaab to
c3c63d3
Compare
| @@ -25,3 +25,13 @@ | |||
| (** ES6 WeakMap API *) | |||
|
|
|||
| type ('k, 'v) t | |||
|
|
|||
| external make : unit -> ('k, 'v) t = "WeakMap" [@@mel.new] | |||
| external get : key:'k Js.dict -> 'v option = "get" [@@mel.send.pipe: ('k, 'v) t] | |||
There was a problem hiding this comment.
hmmm, I guess this could also take a function reference instead of an object, but foregoing that is probably fine for now, as to avoid complicating the API too.
|
Thanks! |
This adds
js_{set,wekset,weakmap}functions copied from https://github.com/rescript-association/rescript-core