ml-in-barcelona / jsoo-react

js_of_ocaml bindings for ReactJS. Based on ReasonReact.
https://ml-in-barcelona.github.io/jsoo-react
MIT License
138 stars 19 forks source link

RFC: Better support for OCaml syntax #67

Closed jchavarri closed 2 years ago

jchavarri commented 2 years ago

After the amazing work done by @davesnx in #66, I think there is just one thing remaining to have a first decent version of jsoo-react to be published in opam: improving the ergonomics when the library and ppx are used with OCaml syntax.

Rendered.

jchavarri commented 2 years ago

One downside of this proposal is that we lose the ability to discern between "dynamic children" vs "static children".

Right now, jsoo-react (like rescript-react) does some magic to allow either of the following:

let t = <Foo> <div /> <div /> </Foo>;

let u = <Foo> <div /> </Foo>;

These translate currently as

let t =
  React.createElementVariadic(
    Foo.make,
    Foo.makeProps(~children=React.null, ()),
    [|
      ReactDom.createDOMElementVariadic("div", [||]),
      ReactDom.createDOMElementVariadic("div", [||]),
    |],
  );

let u =
  React.createElement(
    Foo.make,
    Foo.makeProps(
      ~children=ReactDom.createDOMElementVariadic("div", [||]),
      (),
    ),
  );

In the first case, children prop is set to null, and children as passed as array that will be spread in the createElement call. The second just

This is the trick that allowed to get rid of children spread operator that was used before reason-react v0.7.

With the approach proposed in this RFC, createElement is moved inside the component implementation make function, so we can't know at that point if there are multiple children or just 1.

A couple of possible solutions are:

1) Force component implementations to expose both children and child optional props. So the transformations would work as follow:

```reason
let t = <Foo> <div /> <div /> </Foo>;

let u = <Foo> <div /> </Foo>;

// transforms into
let t =
Foo.make(
    ~children=[
    ReactDom.createDOMElementVariadic("div", [||]),
    ReactDom.createDOMElementVariadic("div", [||]),
    ],
    (),
);
let u = Foo.make(~child=ReactDom.createDOMElementVariadic("div", [||]), ());
```

2) Go back to the original types that reason-react was using, where children would always be a list(element). This means children type is always consistent, but we'd need to bring back the usage of children spread.

I think it makes sense to go with 2, as it's something that has been done in the past already, and also brings the API closer to that in tyxml.

cc @davesnx

jchavarri commented 2 years ago
  1. Go back to the original types that reason-react was using, where children would always be a list(element). This means children type is always consistent, but we'd need to bring back the usage of children spread.

I explored this, but it's fundamentally impossible to use this approach because in any case without leaving behind some of the checks that React do at runtime in dev mode.

As the idea is to move createElement call inside the component implementation, we "lose" the ability to know what kind of children are being passed to every call to make (or createElement), we can only pass the children expression as it comes. So, React will always complain with warning Each child in a list should have a unique even for list literals, where keys are unneeded 😞

jchavarri commented 2 years ago

Implemented in #72.