Re-organise the modules #794
Conversation
| Welcome to the source of ReasonReact! | ||
| ### Welcome to the source of ReasonReact | ||
|
|
||
| We want to expose as minimum modules to the toplevel as possible, and at the same time want to map closely to the npm packages. So each module maps to a npm package, and each sub-module maps to a sub-module. For example: `react-dom` -> `ReactDOM` and `react-dom/server` -> `ReactDOM.Server`. | ||
|
|
||
| Files overview: | ||
|
|
||
| ## Bindings |
There was a problem hiding this comment.
thanks for writing this up. it's very clear.
you may want to check the rendering before merging, as the nested bullets aren't rendered as you'd expect.
| - `ReasonReactRouter`: a simple, yet fully featured router with minimal memory allocations | ||
| - `ErrorBoundary`: component to catch errors within your component tree |
There was a problem hiding this comment.
should this one become React.ErrorBoundary, or do you want it to stay out at the toplevel?
There was a problem hiding this comment.
React.ErrorBoundary sounds nice to keep like it was before, but I dislike the idea of having it unsafe and part of the React module.
I proposed the renaming of SimpleErrorBoundary to make it clearer?
There was a problem hiding this comment.
OK then I'm fine taking the time to think about this, and keeping it ReasonReactErrorBoundary for now?
src/React.re
Outdated
| ([@mel.uncurry] (unit => 'any), ('a, 'b, 'c, 'd, 'e, 'f, 'g)) => 'any = | ||
| "useMemo"; | ||
|
|
||
| /* This is used as return values */ |
There was a problem hiding this comment.
This was for type callback('input, 'output). which is no longer here
| /* This is used as return values */ | |
| /* This is used as return values */ |
There was a problem hiding this comment.
Removed this and added into the React.Uncurried callback
| @@ -195,7 +195,7 @@ external useSyncExternalStore: | |||
| [@mel.module "react"] | |||
| external useSyncExternalStoreWithServer: | |||
| ( | |||
| ~subscribe: (([@mel.uncurry] (unit => unit)) => ([@mel.uncurry] (unit => unit))), | |||
| ~subscribe: ([@mel.uncurry] (unit => unit)) => [@mel.uncurry] (unit => unit), | |||
| @@ -1,6 +1,10 @@ | |||
| (library | |||
| (name react) | |||
| (public_name reason-react) | |||
| (wrapped false) | |||
| ; Explicitly adding modules isn't necessary, but it's a good idea | |||
|
We would need to make a proper release notes with the module changes, but should be small. |
|
@davesnx I'm seeing react-dom/test-utils and react-dom/server getting pulled in by vite during development with this change. Without some sort of change in melange to only import referenced submodules, maybe new modules like |
|
Splitting those into modules might make sense to avoid this issue. Would this problem fade away in production builds, where those modules would be gone with tree-shaking? I would like to now how other bundlers handle dev/prod optimisations like this one, to decide if it's a good solution |
|
I didn't get that far, since vite didn't like the |
CHANGES: * Wrap the `React` library, exposing just a single top-level module (@anmonteiro in [reasonml/reason-react#783](reasonml/reason-react#783)) * Re-organise toplevel modules (@davesnx in [reasonml/reason-react#794](reasonml/reason-react#794)) * Require and adapt to Melange v3 (@anmonteiro in [reasonml/reason-react#821](reasonml/reason-react#821))
As discussed here: #770
This file explains the new organisation of modules: https://github.com/reasonml/reason-react/blob/Unwrap-and-minimize-toplevels/src/README.md
wrapped falseand inlining most of the modules makes the interface a little cleaner for the users. On the other side, we have huge modules (which isn't so bad, but that it just my opinion)Moved all interface from
React.reiinto it's own moduleI didn't took any decision on
ErrorBoundary, neitherReasonReactRouter. I got the feeling that we should rename those asSimpleErrorBoundaryandSimpleRouteror similar. I would be fine to renameReasonReactRoutertoRouter