Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup for document filenames #528

Closed
agarwal opened this issue Apr 21, 2020 · 5 comments
Closed

Cleanup for document filenames #528

agarwal opened this issue Apr 21, 2020 · 5 comments

Comments

@agarwal
Copy link

agarwal commented Apr 21, 2020

I'm still unclear on the file names. We have ReasonReact.re and React.re, and ReactEvent.re and ReactEventRe.re. I'm not sure what the Reason* prefix and *Re suffix indicate if anything. If there is a pattern to the naming, it would be helpful to explain that in the README. Or if the naming is actually inconsistent, then it would be nice to fix it.

@bloodyowl
Copy link
Contributor

That might be the occasion to only expose the current official API with modules without prefix or suffix (moving ReactDOMRe to ReactDOM for instance)

@sgny
Copy link
Contributor

sgny commented Apr 21, 2020

ReasonReact is the old API. Since it's pretty easy to migrate from that to the new API, I don't think it's relevant any longer. I think the documentation is fairly sufficient, but an explicit statement to that effect might be helpful.

Renaming ReactDOMRe to ReactDOM would be really nice.

@rickyvetter
Copy link
Contributor

rickyvetter commented Apr 28, 2020

Yes I agree this is a mess. Will be working to make this more consistent. Here is my current plan. Let me know if any pieces of this don't make sense.

React - continued support
ReactEvent - continued support
ReasonReact - deprecation in 0.9, remove in 1.0
ReasonReactOptimizedCreateClass - see ReasonReact
ReactDOMRe - move to ReactDOM and massive deletion of apis - pending support for bs.as in bs.obj, as well as React.jsx in the ppx and repo
ReactDOMServerRe - see ReactDOMRe
ReasonReactRouter - continued support
ReactEventRe - deprecation in 0.9, remove in 1.0
ReasonReactCompat - see ReasonReact

If all goes to plan this means that in the 1.0 release we have:

React
ReactEvent
ReactDOM
ReactDOMServer
ReasonReactRouter

Where * means that this is just bindings for a JS module or JS functionality. Reason* means that the module carries a Reason runtime.

@agarwal
Copy link
Author

agarwal commented May 24, 2020

In OCaml libraries, I follow these rules:

A single library provides a single namespace, which we realize in OCaml with a module. So there is a single top-level module. Everything else goes under this module. Usually, the top-level module consists strictly of sub-modules, not values or types, i.e. we use dune's auto-wrapping. Less frequently, it makes sense to include values and types also in the top-level module, so you manually write this top-level module. I think the latter is the correct design here. So it makes sense that there is a React module with types like component, etc. But I'm wondering if it would be right to make React.Event, React.DOM, React.DOM.Server instead of ReactEvent, ReactDOM, and ReactDOMServer? It depends also on the corresponding JS names. If ReactEvent, ReactDOM, and ReactDOMServer are consistent with the JS world, then that could be an argument for preferring those names over the dot syntax.

ReasonReactRouter violates one of our other rules, which is to not put the name of the language in the module. It is of course a Reason module, so no reason to state that in the name. It also violates the above rule that a single library provide a single name space. Could this module be called ReactRouter (or as above, React.Router). Are we not calling it ReactRouter because that would confuse it with the JS react-router library? If so, FWIW, that confusion wasn't avoided for me. For some time, until I actually used it, I assumed ReasonReactRouter is a Reason binding to JS's react-router and wondered why it is here instead of a separate bs-react-router library.

Where * means that this is just bindings for a JS module or JS functionality. Reason* means that the module carries a Reason runtime.

Sorry, I didn't understand this. Perhaps it invalidates some of my above points.

@davesnx
Copy link
Member

davesnx commented Jul 10, 2023

This got eventually much better on 0.11, and can be improved with dune.

There's a bit of room for improvement (fe. ReactEvent -> React.Event) which can happen at some point but those breaking changes are hard to sync and bring not much benefit rather than organisation.

@davesnx davesnx closed this as completed Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants