-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
module: remove obsolete global path #1947
Conversation
you should remove the reference to this path in the docs |
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... |
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 ( |
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. |
Related: nodejs/node-v0.x-archive#5286 |
Is this actually causing problems? If not, I don't think we should mess with this unless we know the implications. |
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 |
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... |
@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. |
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.)