Re-organise the modules#794
Conversation
|
|
||
| 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.
| - `ReactTestUtils`: helpers for testing your components | ||
| - `ReasonReactErrorBoundary`: component to catch errors within your component tree | ||
| - `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?
| ([@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
| external useSyncExternalStoreWithServer: | ||
| ( | ||
| ~subscribe: (([@mel.uncurry] (unit => unit)) => ([@mel.uncurry] (unit => unit))), | ||
| ~subscribe: ([@mel.uncurry] (unit => unit)) => [@mel.uncurry] (unit => unit), |
| (name react) | ||
| (public_name reason-react) | ||
| (wrapped false) | ||
| ; Explicitly adding modules isn't necessary, but it's a good idea |
anmonteiro
left a comment
anmonteiro
left a comment
There was a problem hiding this comment.
decide what to name errorBoundary.re -- I propose ReasonReactErrorBoundary to give it a long, unique name.
thanks for working on this
|
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))
3 participants
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