Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should not expose dependencies package's require #15

Open
Stuk opened this issue Mar 14, 2013 · 5 comments
Open

Should not expose dependencies package's require #15

Stuk opened this issue Mar 14, 2013 · 5 comments

Comments

@Stuk
Copy link
Contributor

Stuk commented Mar 14, 2013

In montage we depend on collections, and currently a user can do require("montage/collections"). This shouldn't be possible

@kriskowal
Copy link
Member

That’s debatable, and fixing that emergent pattern would require significant complication. We would need to separate public and private module identifier name spaces and there would be some overlap.

A related issue is that we don’t mimic Node’s module identifier namespace semantics perfectly. That is, in Node, you can only access internal modules with relative module identifiers and dependencies only with top-level module identifiers.

If we solve one inconsistency, we can probably solve both.

@kriskowal
Copy link
Member

It’s worth noting that separating those name spaces would have some impact on how we use module identifiers throughout Montage.

@Stuk
Copy link
Contributor Author

Stuk commented Mar 14, 2013

To add to the debate: dependencies are an implementation detail. One should be able to swap them out without affecting the accesible API.

@kriskowal
Copy link
Member

We should entertain an experiment in v2. We can separate the internal and external name spaces of a package, and instead of normalizing module identifiers to be relative to the package root, decide whether to use the internal or external name space based on whether the identifier starts with . or ... This would make our implementation match the behavior of Node.js, and break everything that uses mr@<2, but would be feasible in the v2 timeline (which I’m holding in suspended animation until further notice).

This would be a pretty significant refactor. Would you up for it any time in the next six months @Stuk?

@Stuk Stuk self-assigned this May 29, 2014
@Stuk
Copy link
Contributor Author

Stuk commented May 29, 2014

I can add it to the plate and see what happens :)

@hthetiot hthetiot assigned hthetiot and unassigned Stuk May 1, 2017
@hthetiot hthetiot added this to the v17.1.x milestone May 1, 2017
@hthetiot hthetiot removed their assignment May 1, 2017
@hthetiot hthetiot modified the milestones: Future, v17.1.x Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants