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

Remove superfluous JS output from ReactDOM.domProps #43

Closed jchavarri closed 2 years ago

jchavarri commented 3 years ago

The domProps "builder" function has a ton of attributes:

https://github.com/jchavarri/jsoo-react/blob/631f9855c727994b12aa928116b9b7068ae15e5c/lib/reactDOM.mli#L65-L75

The resulting JS code generated by jsoo includes zeroes for all of them missing, which is just useless information that increases for every host element rendered (e.g. <div />).

There are some solutions / workarounds that I can think of:

  1. Reduce the amount of attributes (super short term šŸ˜… ) by wrapping <div /> with a "composite" (non host) component like Div, passing only the props the app will use.

  2. "Lift" the object creation up to the ppx, which has already all the information available. So instead of calling domProps here:

https://github.com/jchavarri/jsoo-react/blob/631f9855c727994b12aa928116b9b7068ae15e5c/ppx/ppx.ml#L528-L533

we would do something like:

match nonEmptyProps with
| (Labelled "onClick", expr) :: _ -> (* actually, mapping over the `nonEmptyProps` list, not matching it :) *)
    let mapped_expr = mapper.expr mapper expr in
    [%expr
      let empty = Ojs.empty_obj () in
      Ojs.set empty "onClick"
        (Ojs.fun_to_js 1 (fun inner ->
              [%e mapped_expr] (ReactEvent.Mouse.t_of_js inner))) ;
      Obj.magic empty]
  1. Explore using tyxml, which compiles the attributes down to a list, a representation that doesn't have this issue.
tmattio commented 3 years ago

Thanks a lot for the details on the issue @jchavarri!

It's funny you're mentioning Tyxml, one of the first thing I thought when trying out jsoo-react was "wouldn't it be great if it used Tyxml under the hood" šŸ˜„

Joke aside, it would have several benefits for which I have concrete use cases:

let make ~name =
  (div
     ~className:"text-center mt-12"
     ~children:
       [ (p
            ~className:"text-3xl text-gray-900 mb-4"
            ~children:
              [ React.string
                  ("\240\159\145\139 Welcome " ^ name ^ "! You can edit me in ")
              ; (code
                   ~children:[ React.string "src/components/Greet.re" ]
                   () [@JSX])
              ]
            () [@JSX])
       ; (a
            ~className:"text-3xl no-underline text-blue-500"
            ~href:"https://reasonml.github.io/reason-react/"
            ~children:[ React.string "Learn Reason React" ]
            () [@JSX])
       ]
     () [@JSX])
  [@@react.component]

However I'm not certain Tyxml's ppx matches Reason's JSX ppx.

jchavarri commented 3 years ago

@tmattio yes! Tyxml will definitely make many things more straightforward. I want to release a first version of the library in Opam that remains as close as possible to the existing ReasonReact, to make sure most of the API is covered, and also to facilitate any potential migrations.

But for the version after that, I plan to look in to Tyxml more in depth. The ppxs definitely don't match, but they're not too far either. I started documenting some of the differences between them in https://www.javierchavarri.com/react-server-side-rendering-with-ocaml/.

jchavarri commented 2 years ago

@tmattio we made some exploration to use tyxml, but the divergence is a bit too large, in the end React has a lot of attributes and event handlers that are not HTML. Plus we'd need to move away from the way we handle children etc which means worse support for existing reason-react apps that could be potentially migrated to jsoo-react.

We are working on improvements for OCaml syntax though, you can check RFC #67 to see what we're planning to work on in order to improve OCaml syntax support. Feel free to add any feedback to that PR.