feat: add bindings to Js.Set #1061
Conversation
jscomp/test/js_set_test.ml
Outdated
There was a problem hiding this comment.
Are these tests useful, or is it better to move them to test/blackbox-tests/js_set.t ? Seems like I couldn't make them to fail 🤔
There was a problem hiding this comment.
Looks like they're running on CI :)
There was a problem hiding this comment.
Ah, they'll be compiled and promoted if you comment this stanza https://github.com/melange-re/melange/blob/main/jscomp/dune.
| external fromArray : 'a array -> 'a t = "Set" [@@mel.new] | ||
| external toArray : 'a t -> 'a array = "Array.from" | ||
| external size : 'a t -> int = "size" [@@mel.get] | ||
| external add : value:'a -> 'a t = "add" [@@mel.send.pipe: 'a t] |
There was a problem hiding this comment.
I'm not sure returning 'a t is a good idea. I guess you did it to have a nice pipe API, but on the JS side the set is actually mutated, and this API doesn't make it obvious. See https://github.com/melange-re/melange/pull/1061/files#diff-64db361f42339ab2d557daeaff34549e0d79fc297dfc2411317c6d64f2e6557bR33-R41
There was a problem hiding this comment.
Indeed, but the documentation mentions the return value is the set indeed. So we'd be lying by pretending it's unit even if that makes it more obvious that the set was mutated, no? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/add#return_value
test/blackbox-tests/js_set.t
Outdated
| > let t_1 = Js.Set.make () | ||
| > let t_2 = Js.Set.add ~value:1 t_1 | ||
| > let () = Js.log (Printf.sprintf "t_1 size = %i; t_2 size = %i" (Js.Set.size t_1) (Js.Set.size t_2)) | ||
| > EOF | ||
| $ dune build @melange | ||
| $ node _build/default/out/x.js | ||
| true | ||
| false | ||
| t_1 size = 1; t_2 size = 1 |
There was a problem hiding this comment.
I would not expect the size of t_1 to be 1 with such API. Or am I missing something ?
There was a problem hiding this comment.
depends whether you think that what you have is a reference or a value. In OCaml, Set and Map are values, while here when binding to JS it's a reference (like e.g.ref or Queue.t etc.) which is mutated underneath. So the test looks correct to me.
|
CI fails due to |
Yeah, I mentioned this in #1047 (comment). It's fine to just comment them out for now. |
|
merged in #1047 |
@anmonteiro I couldn't push directly to your branch, so I created a new one with the bindings improvements