reasonml / reason-react

Reason bindings for ReactJS
https://reasonml.github.io/reason-react/
MIT License
3.25k stars 349 forks source link

Re-organise the modules #794

Closed davesnx closed 1 year ago

davesnx commented 1 year ago

As discussed here: https://github.com/reasonml/reason-react/issues/770

This file explains the new organisation of modules: https://github.com/reasonml/reason-react/blob/Unwrap-and-minimize-toplevels/src/README.md

davesnx commented 1 year ago

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

joprice commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.