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

Fix path to global node_modules on Windows #5

Merged
merged 1 commit into from
Apr 7, 2016

Conversation

rmbaad
Copy link
Contributor

@rmbaad rmbaad commented Apr 2, 2016

process.execPath return path to instlled nodejs on Windows, but not to global node_modules directory.
E.g. package npmls use global-prefix for getting path to node_modules, but it's returned wrong path.

@jorrit
Copy link
Contributor

jorrit commented Apr 5, 2016

I think you should squash the commits.

@jonschlinkert
Copy link
Owner

@jorrit I can squash the commits on merge, no problem. However, @kuksikus can you share any additional insight into when APPDATA would be used for the prefix this way?

Since npm itself seems to only use APPDATA or user home for cache, and I can't find any code in npm's codebase that falls back to the path you've defined, I just want to make sure we're correctly setting the variable.

thanks!

@rmbaad
Copy link
Contributor Author

rmbaad commented Apr 6, 2016

@jonschlinkert I found the problem with path on Windows, and found this path in process.env while debugging.

A quote from the npm wiki:

By default, npm is installed alongside node in C:\Program Files (x86)\nodejs. npm's globally installed packages (including, potentially, npm itself) are stored separately in a user-specific directory (which is currently C:\Users\AppData\Roaming\npm). Because the installer puts C:\Program Files (x86)\nodejs before C:\Users\AppData\Roaming\npm on your PATH, it will always use version of npm installed with node instead of the version of npm you installed using npm -g install npm@

If I try "npm ls -g" I see "C:\Users\jibon\AppData\Roaming\npm" and list packages.
And command "npm config ls -l" return

; builtin config undefined
prefix = "C:\Users\jibon\AppData\Roaming\npm"

@jonschlinkert
Copy link
Owner

thanks for the detail! I'll merge sometime today

@tadatuta
Copy link

tadatuta commented Apr 6, 2016

👍

@rmbaad
Copy link
Contributor Author

rmbaad commented Apr 7, 2016

I squashed commits

@jorrit
Copy link
Contributor

jorrit commented Apr 7, 2016

Isn't it an idea to just read the output of npm config get prefix?

[edit]
When scripts are run from npm, the npm_config_prefix env variable is also present. That could also be used before executing npm.

@jonschlinkert
Copy link
Owner

Isn't it an idea to just read the output of npm config get prefix?

yeah I was thinking of doing something like that to get custom prefixes.

@rmbaad
Copy link
Contributor Author

rmbaad commented Apr 7, 2016

I think it's a bad idea, beacause npm config get is slow. Just try time npm config get prefix

@jorrit
Copy link
Contributor

jorrit commented Apr 7, 2016

I think it should only be executed once and cached in global-prefix from that moment on.

@jonschlinkert
Copy link
Owner

I think it's a bad idea, beacause npm config get is slow. Just try time npm config get prefix

that's why I haven't done it yet lol, I mentioned how slow it is somewhere else. I'm very open to suggestions. However, it's also ridden with edge cases apparently. Any suggestions?

@jonschlinkert
Copy link
Owner

I don't think caching it is a good idea if you consider some common use cases for custom prefixes

@jorrit
Copy link
Contributor

jorrit commented Apr 7, 2016

You mean that the prefix changes while the application is running?

@jonschlinkert
Copy link
Owner

You mean that the prefix changes while the application is running?

yeah you're probably right. I'm overcomplicating it.

fwiw I almost always cache file paths anyway:

the closest lib I've found to what I'd like to use is https://www.npmjs.com/package/rc. seems like a nice lib, but it seems like swatting a fly with a hammer in this case. Preferably someone knows of a lib that simply resolves custom prefixes. If not I might create one

@rmbaad
Copy link
Contributor Author

rmbaad commented Apr 7, 2016

I think custom prefixes it's a different story. AFAIK current lib doesn't work with default prefixes on Windows. Maybe solve this problem at first? :)

@jorrit
Copy link
Contributor

jorrit commented Apr 7, 2016

@kuksikus I think you're right. The current patch greatly improves the current situation.

@jonschlinkert jonschlinkert merged commit 39bca9a into jonschlinkert:master Apr 7, 2016
@jonschlinkert
Copy link
Owner

sorry for the delay. thanks @kuksikus, published and released as 0.1.3

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.

None yet

4 participants