Re-organise the modules #794

davesnx · 2023-10-11T04:06:30Z

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 false and 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.rei into it's own module
I didn't took any decision on ErrorBoundary, neither ReasonReactRouter. I got the feeling that we should rename those as SimpleErrorBoundary and SimpleRouter or similar. I would be fine to rename ReasonReactRouter to Router

anmonteiro · 2023-10-12T00:30:29Z

src/README.md

-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


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.

anmonteiro · 2023-10-12T00:30:54Z

src/README.md

 - `ReasonReactRouter`: a simple, yet fully featured router with minimal memory allocations
+- `ErrorBoundary`: component to catch errors within your component tree


should this one become React.ErrorBoundary, or do you want it to stay out at the toplevel?

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?

OK then I'm fine taking the time to think about this, and keeping it ReasonReactErrorBoundary for now?

Sounds perfect

anmonteiro · 2023-10-12T00:38:04Z

src/React.re

+  ([@mel.uncurry] (unit => 'any), ('a, 'b, 'c, 'd, 'e, 'f, 'g)) => 'any =
+  "useMemo";
+
+/* This is used as return values  */


This was for type callback('input, 'output). which is no longer here

Suggested change

/* This is used as return values */

/* This is used as return values */

Removed this and added into the React.Uncurried callback

anmonteiro · 2023-10-12T00:40:59Z

src/React.rei

@@ -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),


refmt did this?

Tested and looks fine: https://melange.re/unstable/playground/?language=Reason&code=W0BtZWwubW9kdWxlICJyZWFjdCJdCmV4dGVybmFsIHVzZVN5bmNFeHRlcm5hbFN0b3JlOgogICgKICAgIH5zdWJzY3JpYmU6IChbQG1lbC51bmN1cnJ5XSAodW5pdCA9PiB1bml0KSkgPT4gW0BtZWwudW5jdXJyeV0gKHVuaXQgPT4gdW5pdCksCiAgICB%2BZ2V0U25hcHNob3Q6IHVuaXQgPT4gJ3NuYXBzaG90LAogICAgfmdldFNlcnZlclNuYXBzaG90OiBbQG1lbC51bmN1cnJ5XSAodW5pdCA9PiAnc25hcHNob3QpCiAgKSA9PgogICdzbmFwc2hvdCA9CiAgInVzZVN5bmNFeHRlcm5hbFN0b3JlIjsKCmxldCBnZXRTdGF0ZSA9ICgpID0%2BIDIyOwpsZXQgc3Vic2NyaWJlV2l0aE1vY2sgPSBsaXN0ZW5lciA9PiB7CiAgbGV0IF8gPSBsaXN0ZW5lcigpOwogIEpzLmxvZygiY29zaXRhcyIpOwogICgpID0%2BICgpOwp9OwoKbGV0IHNuYXBzaG90ID0KICB1c2VTeW5jRXh0ZXJuYWxTdG9yZSh%2Bc3Vic2NyaWJlPXN1YnNjcmliZVdpdGhNb2NrLCB%2BZ2V0U25hcHNob3Q9Z2V0U3RhdGUpOwo%3D&live=off

anmonteiro · 2023-10-12T00:42:36Z

src/dune

@@ -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


great idea 👍

anmonteiro

looking very nice

anmonteiro

decide what to name errorBoundary.re -- I propose ReasonReactErrorBoundary to give it a long, unique name.

thanks for working on this

davesnx · 2023-10-13T08:19:36Z

We would need to make a proper release notes with the module changes, but should be small.

joprice · 2023-10-15T22:09:33Z

@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 ReactDOMServer and ReactDOMTestUtils are necessary to avoid this?

davesnx · 2023-10-15T23:33:13Z

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

joprice · 2023-10-16T01:02:38Z

I didn't get that far, since vite didn't like the require calls in the server-side commonjs modules. Not sure these particular ones will be shaken away, since I think purity of the module affect tree-shaking as well and would have to check that. I think either way, it's unexpected to pull in test utils or, in general, something you didn't intend to import, even in dev.

@anmonteiro

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))

davesnx added 2 commits October 11, 2023 06:06

Re-organise the modules

e3325fa

Forgot to promote cram tests

5979db5

anmonteiro reviewed Oct 12, 2023

View reviewed changes

davesnx added 2 commits October 12, 2023 16:32

Fix readme for src/readme

68d4c2a

Annotate callback for Uncurried

1cfc794

anmonteiro approved these changes Oct 12, 2023

View reviewed changes

davesnx added 2 commits October 13, 2023 10:17

Rename errorBoundary to ReasonReactErrorBoundary

6c1794a

Add changes item

dc1a89a

Fix test case for ReasonReactErrorBoundary

6c4d5f4

davesnx merged commit 3ab8747 into main Oct 13, 2023

anmonteiro deleted the Unwrap-and-minimize-toplevels branch October 13, 2023 22:36

jchavarri mentioned this pull request Oct 24, 2023

update to reason-react changes in event ml-in-barcelona/server-reason-react#83

Merged

davesnx mentioned this pull request Feb 10, 2024

[new release] reason-react (2 packages) (0.14.0) ocaml/opam-repository#25227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-organise the modules #794

Re-organise the modules #794

davesnx commented Oct 11, 2023 •

edited

Loading

anmonteiro Oct 12, 2023

anmonteiro Oct 12, 2023

davesnx Oct 12, 2023

anmonteiro Oct 12, 2023

davesnx Oct 13, 2023

anmonteiro Oct 12, 2023

davesnx Oct 12, 2023

anmonteiro Oct 12, 2023

davesnx Oct 12, 2023

davesnx Oct 12, 2023

anmonteiro Oct 12, 2023

anmonteiro left a comment

anmonteiro left a comment

davesnx commented Oct 13, 2023

joprice commented Oct 15, 2023

davesnx commented Oct 15, 2023

joprice commented Oct 16, 2023

		- `ReasonReactRouter`: a simple, yet fully featured router with minimal memory allocations
		- `ErrorBoundary`: component to catch errors within your component tree

	/* This is used as return values */
	/* This is used as return values */

Re-organise the modules #794

Re-organise the modules #794

Conversation

davesnx commented Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmonteiro left a comment

Choose a reason for hiding this comment

anmonteiro left a comment

Choose a reason for hiding this comment

davesnx commented Oct 13, 2023

joprice commented Oct 15, 2023

davesnx commented Oct 15, 2023

joprice commented Oct 16, 2023

davesnx commented Oct 11, 2023 •

edited

Loading