LexiFi / gen_js_api

Easy OCaml bindings for Javascript libraries
MIT License
177 stars 31 forks source link

Support boolean "enum"s and boolean union discriminators #127

Closed cannorin closed 3 years ago

cannorin commented 3 years ago

TypeScript allows boolean values to be used as union discriminators since it allows Boolean Literal Types. It's better to support boolean union discriminators in gen_js_api too.

Note there is a real-world example of such union types: https://github.com/microsoft/TypeScript/blob/master/lib/typescript.d.ts#L5857-L5874

Also, TS allows an union of boolean values and other literal types for the same reason:

type Foo = true | 42

Unions of literal types like above should easily be mapped to OCaml with gen_js_api's [@js.enum]. Although JS and TS do not have "boolean enums", it is convenient to just pretend as if they do and add boolean support to [@js.enum].

With this PR merged, the union type shown above can be mapped as:

type foo =
  | Foo_true [@js true]
  | Foo_42   [@js 42]
  [@@js.enum]

and should generate the following:

type foo =
  | Foo_true 
  | Foo_42 
let rec (foo_of_js : Ojs.t -> foo) =
  fun x2 ->
    let x3 = x2 in
    match Ojs.type_of x3 with
    | "number" ->
        (match Ojs.int_of_js x3 with | 42 -> Foo_42 | _ -> assert false)
    | "boolean" ->
        (match Ojs.bool_of_js x3 with | true -> Foo_true | _ -> assert false)
    | _ -> assert false
and (foo_to_js : foo -> Ojs.t) =
  fun x1 ->
    match x1 with
    | Foo_true -> Ojs.bool_to_js true
    | Foo_42 -> Ojs.int_to_js 42

TODO:

cannorin commented 3 years ago

Note that the test in #125 should be updated to reflect the addition of boolean union discriminators.

Also there are many redundant match cases in the current implementation like this: https://github.com/LexiFi/gen_js_api/blob/28b43a33037a29cbe6e91f07600f4f432d73c778/ppx-test/expected/union_and_enum.ml#L288-L291

I think I can do another PR to eliminate these kind of dead match cases (and it will solve the above problem too). What do you think?

alainfrisch commented 3 years ago

Thanks for the contribution, which makes perfect sense. I've pushed some commits to your branch, to simplify a bit the code around the changes in this PR (even if not strictly related to it). Do you agree with those changes?

cannorin commented 3 years ago

Yes, thanks 🙂