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

Inline props as object #66

Closed davesnx closed 2 years ago

davesnx commented 2 years ago

Fixes #43.

This PR changes the interface for React.Dom.domProps. Before these changes, domProps was a function with ~400 optional labelled arguments, after these changes we treat it as an object.

There are a few reasons for this change, but the most important is bundle size. jsoo compiles the optional arguments into zeros generating per each domProps call near 400 zeros, jsoo treated labeled arguments as positions into arrays. There are a few issues opened about this, I only found this one though: https://github.com/ocsigen/js_of_ocaml/issues/801

Another of the reasons is about correctness, props in React are an object and after those changes, we treat it as an Object as well. Giving the same flexibility as the underneath layer could enable features that aren't possible on reason-react or rescript/react, for example adding missing attributes, allowing destructuring props in bindings, allowing "data"-attributes or type-safety on certain attributes.

Todos

Doubts

I have a doubt about the ref prop. Right now the ref is typed with React.Dom.Ref.t option but React.Dom.domRef doesn't seem to return an optional. But I believe the bindings are not the same as reason-react since they use an anonymous type from the useRef.

File "example/src/Code.re", line 14, characters 14-45:
14 |     <code ref={React.Dom.Ref.domRef(codeRef)}> {text |> React.string} </code>
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This expression has type React.Dom.domRef
       but an expression was expected of type React.Dom.domRef option

Future

As mentioned before, this change allows us a few niceties on the dom attributes API, from the top of my head:

PS: I have the editor configured to format using ocamlformat, I believe we should run it across all files (I separate the formatting into one commit, in order to not affect the review)

jchavarri commented 2 years ago

This is a quick analysis of bundle size impact of this PR using webpack-bundle-analyzer.

For each case, I ran:

This branch (https://github.com/reason-in-barcelona/jsoo-react/pull/66/commits/84e7869ba23ec3c7e9be85233cc4da4318a85920):

image

Master branch (https://github.com/reason-in-barcelona/jsoo-react/commit/1ff26837c94142005a61fea5316df0028529132d):

image
jchavarri commented 2 years ago

Bringing this in. Thanks again @davesnx !