Bindings to external JS components

[jchavarri]

Fixes #77.

Adds support to bind to existing React.js components, either in app or in libraries:

OCaml syntax:

external%component make : name:Js.js_string Js.t -> React.element
  = {|require("./external").Greeting|}

Reason syntax:

[@react.component]
    external make: (~name: Js.t(Js.js_string)) => React.element =
      {|require("./external").Greeting|};

For now, there is no processing of types with gen_js_api or otherwise. In the future if using JavaScript types directly becomes too cumbersome, we can introduce it somehow.

[jchavarri]

@zbaylin I think this should be ready. I updated description with basic usage in both syntaxes, and added some tests for both ppx and functionality.

I will leave the PR opened a few more days to do some testing, if you end up taking the PR for a spin and find anything not working ok please let us know! :)

[jchavarri]

Gonna merge this, I haven't had the chance to test it in real apps yet, but the tests in PR cover the functionality.

[glennsl]

Trying this out now, and am confused by several things:

1. There is no terminating unit argument on the external, but it's still required when invoking it (in OCaml). And omitting it yields a nasty error message.
2. It's not clear how to accept children. I thought it might just be adding an argument children:React.element Js.js_array Js.t, but that results in a runtime error: Objects are not valid as a React child (found: object with keys {$$typeof, render}). If you meant to render a collection of children, use an array instead.
3. Emacs locks up when the cursor is in the vicinity of one of the externals. No idea what that's about, I've never experienced it with anything else, but it happens consistently with this one definition:

external%component tile_layer : attribution:Js.js_string Js.t -> url:Js.js_string Js.t -> React.element
  = {|require("react-leaflet").TileLayer|}

Also, it would be nice to not have to deal with Js types directly. The external has all the type information, so it should be pretty easy to have the PPX wrap string in Js.string etc., I hope?

[wokalski]

@glennsl ppx doesn't have access to the type information so the wrapping would have to happen based on syntactic analysis. There's a convenience/correctness tradeoff

[glennsl]

The type annotations are required by the external syntax and so will always be directly available. Of course they could also be type aliases or type variables, but the chance of having a string annotation that isn't actually Stdlib.string, for example, seems so low that I think it can be safely ignored.

[jchavarri]

1. There is no terminating unit argument on the external, but it's still required when invoking it (in OCaml). And omitting it yields a nasty error message.

I believe this is general problem with the ppx (I mean, not only externals, also regular components). The ppx currently "swallows" function types even if they don't end with unit, but unit is necessary as there are optional key and ref. I will open a new issue to track this.

2. It's not clear how to accept children. I thought it might just be adding an argument children:React.element Js.js_array Js.t, but that results in a runtime error: Objects are not valid as a React child (found: object with keys {$$typeof, render}). If you meant to render a collection of children, use an array instead.

Hm, this is strange. Can you share some repro code that I can use? No need to set up project, just js React library or code that triggers + OCaml code to build the repro.

3. Emacs locks up when the cursor is in the vicinity of one of the externals. No idea what that's about, I've never experienced it with anything else, but it happens consistently with this one definition:

This could be due to ppx locations. I opened related issue a few days ago an issue that I believe is caused by this as well https://github.com/ml-in-barcelona/jsoo-react/issues/121. Yesterday I spent a couple of hours looking into it, but I have no clue how to debug this stuff, is the problem merlin? ocamllsp-server? :(

I will try to repro, but I use vscode.

Also, it would be nice to not have to deal with Js types directly. The external has all the type information, so it should be pretty easy to have the PPX wrap string in Js.string etc., I hope?

This is a very slippery slope. Ultimately, it is essentially recreating gen_js_api, but in the library. I think for v1 we can keep adding JS types to the extension and then either convert manually, or use gen_js_api to convert to friendly types. What do you think?

[glennsl]

I will open a new issue to track this.

Thanks!

Can you share some repro code that I can use

Repro here: https://github.com/glennsl/jsoo-react-external-children-repro

is the problem merlin? ocamllsp-server?

I use merlin, but it happens even with merlin-mode disabled (it is by default), which lead me to believe it might just be the syntax support. I also haven't encountered the problem while creating the repro on my home computer, so might be something with my work setup. I'll see if I can narrow it down a bit more.

This is a very slippery slope. Ultimately, it is essentially recreating gen_js_api, but in the library. I think for v1 we can keep adding JS types to the extension and then either convert manually, or use gen_js_api to convert to friendly types. What do you think?

I'm not sure how this would interact with gen_js_api, but if that's relatively easy and straightforward then sure! Could you give an example of how that would look?

It just seemed like it should be very simple and easy to wrap strings in Js.string, bools in Js.bool, arrays in Js.array and leave everything else as-is, given that the types are right there. Not sure what would be so slippery about it.

[jchavarri]

Repro here: https://github.com/glennsl/jsoo-react-external-children-repro

Thanks for the repro. I tracked this down, and the issue seems to be the generated code for the TileLayer component. The JavaScript code generated is somehow like this:

       var tile_layer=
        function(Props)
         {return caml_call1(require("react-leaflet").TileLayer,Props)},

But require("react-leaflet").TileLayer is not a function of arity 1. It's not even a function 😅

Here is the relevant code from ppx tests:

https://github.com/glennsl/jsoo-react/blob/03dca8904dab889cb0c2622295ac421c602ebdd5/ppx/test/pp_ocaml.expected#L272-L278

I made a huge assumption when implementing externals. I tried to reuse as much as possible of existing ppx, for components written in-app, but the ppx assumes that all React components are plain functions that take one param, but this is not the case (at all) when interfacing with JavaScript components.

I will try to look into this, but not sure when it'll be possible. We might need to rethink the way externals are built :( sorry about that!

[jchavarri]

I reported the issue and draft a potential solution in https://github.com/ml-in-barcelona/jsoo-react/issues/123, does not look that bad.

[jchavarri]

I'm not sure how this would interact with gen_js_api, but if that's relatively easy and straightforward then sure! Could you give an example of how that would look?

I am thinking st like this (not using any jsoo-react ppx transform, just what I would use if I wanted to convert with gen_js_api):

include [%js:
  type js_comp_props
  val make_props:
    ?key:string ->
    text:string ->
    click:(unit -> unit) ->
    unit -> js_comp_props
    [@@js.builder]
]

type js_comp
let js_comp : js_comp = Js_of_ocaml.Js.Unsafe.js_expr {|require("my-lib").Component|}

let make ?key ~text ~click () = React.createElement js_comp (make_props ?key ~text ~click)

I haven't tested any of this though, it's just a quick idea.

It just seemed like it should be very simple and easy to wrap strings in Js.string, bools in Js.bool, arrays in Js.array and leave everything else as-is, given that the types are right there. Not sure what would be so slippery about it.

What I mean with slippery is that you have to make sure you convert array, but also the type passed as variable of the array, you can also have nested arrays, or optional values. Maybe you want to use some tuples and convert to JS arrays, who knows. You might need to pass some callbacks to these JS components in the future, with all the arity shenanigans.

It's not that straightforward in my experience. The good news is that gen_js_api already deals with all this stuff so maybe there is a way to use it as a "ppx library" from jsoo-react ppx, so we just pass it the external ast node and we get back everything needed, if the naif example shown above makes sense.

[glennsl]

Thanks for looking into it and finding a possible solution. I can have a go at implementing this right away.

I am thinking st like this (not using any jsoo-react ppx transform, just what I would use if I wanted to convert with gen_js_api):

Thanks for the example! This will be very useful if I have to fall back to using gen_js_api, but also to think about how this could integrate with the external PPX.

What I mean with slippery is that you have to make sure you convert array, but also the type passed as variable of the array, you can also have nested arrays, or optional values. Maybe you want to use some tuples and convert to JS arrays, who knows. You might need to pass some callbacks to these JS components in the future, with all the arity shenanigans.

I think if we just do first order conversions we'd cover 90% of use cases (strings alone are probably 80%). Doing second order conversions, such as string array, tuples etc., would cover maybe 95%. I can't really imagine a use case for options, since that's just an optional prop, but callbacks are indeed both useful and tricky. It would still be immensely useful without callback support though, the first- and second-order conversions mentioned seems pretty easy to implement, and explaining it can be done with just a short list of supported conversions.

I agree it would be much better to be able to use the full power of gen_js_api, of course, but unfortunately I have no idea how to do that :/