Skip to content

Conversation

jchavarri
Copy link
Member

I think the "vendored" definitions of Unsafe, number_of_float etc. were there for historical reasons, as it was hard to consume jsoo as a library. But that's not the case anymore.

@jchavarri jchavarri requested a review from anmonteiro July 7, 2023 09:05
@hhugo
Copy link
Contributor

hhugo commented Jul 7, 2023

You might want to use js_of_ocaml-compiler.runtime which expose Jsoo_runtime.

@jchavarri
Copy link
Member Author

Thanks for the tip @hhugo. I tried to use that approach, but then the type of Js.t changes, from having one argument, to zero arguments. So it becomes impossible to express types like Js.js_string Js.t.

Should I just replace Js.js_string Js.t with Js.t? If yes, what are the consequences? I am not sure I understand.

@jchavarri
Copy link
Member Author

@hhugo
Copy link
Contributor

hhugo commented Jul 7, 2023

Js.t? If yes, what are the consequences? I am not sure I understand.

It depends on what you want to do. Jsoo_runtime was created so that gen_js_api and brr can use it without the "jsoo typing" layer.
If you want to rely on the jsoo (object based) typing, keep the current approach. Jsoo_runtime is merely a collection of external to interact with the jsoo runtime.

@jchavarri
Copy link
Member Author

Hm, I see, thx. We rely on values like undefined as well as some of the functions in Js.Unsafe. For example, Js.Unsafe provides functions to create JS objects with the values having multiple types, e.g.:

let t =
  Js.Unsafe.(
    obj [| ("js_error_msg", inject (Js.string "foo")); ("row", inject 2) |])

But this fails to build when using Jsoo_runtime:

5 | let t = Js.(obj [| ("js_error_msg", Js.string "foo"); ("row", 2) |])
                                                                  ^
Error: This expression has type int but an expression was expected of type
         Js.t

@hhugo
Copy link
Contributor

hhugo commented Jul 7, 2023

let t = Js.(obj [| ("js_error_msg", Js.string "foo"); ("row", Obj.magic 2) |])

or

let t = Js.(obj [| ("js_error_msg", Js.string "foo"); ("row", Js.number_of_float 2.) |])

@hhugo
Copy link
Contributor

hhugo commented Jul 7, 2023

undefined would be Js.pure_js_expr "undefined"

@jchavarri jchavarri force-pushed the playground/use-jsoo branch from d367597 to 8df733b Compare July 7, 2023 15:34
@jchavarri
Copy link
Member Author

Thanks, I applied the suggested changes. I am not fond of the 5 appearances of Obj.magic but I guess it's the same as using Unsafe.inject, essentially.

playground: use js_of_ocaml-compiler.runtime

wip
@anmonteiro anmonteiro force-pushed the playground/use-jsoo branch from 596c4a2 to 6ef37bf Compare July 7, 2023 22:43
@anmonteiro anmonteiro merged commit fb6dae8 into main Jul 8, 2023
@anmonteiro anmonteiro deleted the playground/use-jsoo branch July 8, 2023 00:40
jchavarri added a commit that referenced this pull request Jul 10, 2023
* main: (22 commits)
  playground: fix warning errors
  playground: add test for bad error msg
  playground: use js_of_ocaml library (#654)
  chore: update nix flakes (#652)
  playground: bring back cmijs hack (#648)
  Update CONTRIBUTING.md to include an installation step for melange.opam and a link to the Pull Request instructions (#651)
  playground: specify ppx dep (#646)
  playground: package + es6 + direct reason compilation (#632)
  feat: add `-rectypes` (#644)
  fix changelog
  fix: spurious warning 61 in externals
  test: show spurious warning 61 in externals
  test: show how to disable ppx alerts (#642)
  fix: disable warning 61 (`unboxable-type-in-prim-decl`) for deriving abstract externals
  test: show spurious warning on single field records with [@@deriving abstract]
  chore: format runtime / belt / node libraries (#640)
  chore: test on OCaml 5.1 (#639)
  chore: require dune 3.8, format, cleanup (#638)
  refactor: Don't use `(stdlib ...)` for the `Js.*` runtime (#637)
  chore: snapshot JS test files (#636)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants