Skip to content

feat: reduce too many exposes module in toplevel#773

Closed
r17x wants to merge 3 commits into
reasonml:mainfrom
r17x:feat/reduce-too-many-toplevel-module
Closed

feat: reduce too many exposes module in toplevel#773
r17x wants to merge 3 commits into
reasonml:mainfrom
r17x:feat/reduce-too-many-toplevel-module

Conversation

@r17x

@r17x r17x commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

Based on this #770

@davesnx

davesnx commented Sep 6, 2023

Copy link
Copy Markdown
Member

Instead of going to that direction, probably (wrapped false) is a smarter move?

@r17x

r17x commented Sep 6, 2023

Copy link
Copy Markdown
Contributor Author

@davesnx I'm not familiar with term of (wrapped false) could you explain about it?

@davesnx

davesnx commented Sep 6, 2023

Copy link
Copy Markdown
Member

wrapped false is the default behaviour from ReScript where each module is exposed so given a library like reason-react you will expose all modules (React, ReactDOM, ReasonReactRouter, etc)

With wrapped true you will expose this library under a name (or other more exotic forms) so lets say I name this library React, I could expose all modules under React now: (React.React, React.ReactDOM, React.ReasonReactRouter, etc)

@anmonteiro

Copy link
Copy Markdown
Member

Instead of going to that direction, probably (wrapped false) is a smarter move?

We'll need both. This PR alone still exposes all the other modules because there's (wrapped false) in the dune file.

Comment thread src/dune
(preprocess
(pps melange.ppx reason-react-ppx))
(wrapped false)
(wrapped true)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been changes to be true @davesnx . I think is incomplete yet? could you give me suggestions for this changes?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to move forward with this would be to fix the build

@anmonteiro

Copy link
Copy Markdown
Member

@davesnx do you want to include this for 0.12, or should it be included after that?

@davesnx

davesnx commented Sep 11, 2023

Copy link
Copy Markdown
Member

Let's do it for 0.13

@anmonteiro

Copy link
Copy Markdown
Member

This is actually going to be a lot harder than initially thought, since many modules depend on the React module.

@anmonteiro

Copy link
Copy Markdown
Member

solution in #783

@r17x

r17x commented Sep 15, 2023

Copy link
Copy Markdown
Contributor Author

I learn a lot from #783 and I closed this PRs

Good job!

Thanks @anmonteiro

@r17x r17x closed this Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants