Skip to content

Conversation

@Trott
Copy link
Member

Resolves#1946

(One test is failing but it's failing for me without this change too and it looks like it has to do with #1945. If I remove that test, everything else passes make test with no problem.)

@brendanashworthbrendanashworth added the module Issues and PRs related to the module subsystem. label Jun 11, 2015
@targos
Copy link
Member

you should remove the reference to this path in the docs

@Trott
Copy link
MemberAuthor

Thanks. I just pushed a commit to update the docs. I'm not sure what the historical reasons (mentioned in the docs) are for that path, so I'm not sure if removing it is a good idea or not, but I'll leave this open for other more informed opinions. If it's a horribly misguided idea, don't hesitate to close...

@TrottTrott changed the title module: remove invalid global pathmodule: remove obsolete global pathJun 11, 2015
@Trott
Copy link
MemberAuthor

I suppose we don't actually know how many installations out there use that path for whatever reason. Therefore, I imagine this would actually have to get a deprecation warning first, and then be removed at some future major version bump. Which: Ugh. I'm game for that if there's a general consensus that it's A Good Thing, but I guess I'd also want to remove the other two semi-magic kinda-global paths in there too ($HOME/.node_modules and $HOME/.node_libraries).

@gaspard
Copy link

I am all for removing the "semi magic" paths. It makes things confusing and as the docs say, can lead to wrong behaviour when the incorrect module is loaded by accident. If require behaviour is frozen and these global paths loading idiom are not even listed on the pseudo-code, they should belong to "deprecated" and be gone by next major semver version change.

@silverwind
Copy link
Contributor

Related: nodejs/node-v0.x-archive#5286

@cjihrig
Copy link
Contributor

Is this actually causing problems? If not, I don't think we should mess with this unless we know the implications.

@trevnorris
Copy link
Contributor

agreed. unless there's an actual issue, and preferably a test to reproduce, then this is something that we should leave alone. I've been surprised in the past how seemingly innocuous changes to the module system have caused non trivial issues for more than a few people.

/cc @isaacs

@Trott
Copy link
MemberAuthor

The closest I can come up with to a real problem this causes is #1940. Which isn't really much of a problem unless it trips up people frequently. So I'm inclined to close this and forget about it unless it starts coming up again and again. Which is what I'm going to do now...

@TrottTrott closed this Jun 11, 2015
@trevnorris
Copy link
Contributor

@Trott Thanks for you work on this. If Isaac responds he can give a more definitive answer to whether this would be an issue or not.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

moduleIssues and PRs related to the module subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@Trott@targos@gaspard@silverwind@cjihrig@trevnorris@brendanashworth