search

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

dune: remove `(wrapped false)` #45

Closed jchavarri closed 3 years ago

jchavarri commented 3 years ago

From @rgrinberg in the OCaml discord:

(wrapped false) is a pure compatibility hack for libraries before the dune world. We needed to support existing libraries from module aliases were a thing. No new libraries should use it

I'm a bit concerned about compatibility with current ReasonReact. Moving to an entry point module called React, that then holds the other modules like React.Dom, React.Event is nicer (imo) but also breaks compatibility with existing components in RR. I wonder if this will be a real issue for those trying to copy/paste code. @tmattio I wonder what you think?

tmattio commented 3 years ago

Personally, breaking compatibility with ReasonReact is not going to be a blocker, and React.Dom, React.Event, etc. also seems nicer to me.

One idea that could give you more freedom on the design of the API would be to introduce a Compat module that follows ReasonReact API more closely. You'd have the best of both worlds: the top-level module could deviate from ReasonReact a bit to follow OCaml's conventions more closely (I am assuming this is the target users) while ensuring that users can copy-paste ReasonReact components in their projects (by adding an open React.Compat statement on top).

jchavarri commented 3 years ago

Thanks for the feedback! The Compat module sounds like a good approach, I'll see how these migrations turn out in practice —as there's also pipe first operator, modules that are BS specific, etc.— and if it makes sense, will add it.