jonschlinkert / global-prefix

Get the npm global path prefix. Same code used internally by npm.
MIT License
28 stars 12 forks source link

Discussion to improve reliability on Windows #21

Open veljkoz opened 6 years ago

veljkoz commented 6 years ago

This line:

https://github.com/jonschlinkert/global-prefix/blob/49684bdf64fd6e5f11dc7caac8c6eb9abfd376b9/index.js#L57

//...
if (isWindows()) {

    // c:\node\node.exe --> prefix=c:\node\

    prefix = process.env.APPDATA

      ? path.join(process.env.APPDATA, 'npm')

      : path.dirname(process.execPath);

  } else {
//...

It would've give proper results without assuming the npm is under the APPDATA, i.e. if it's just:

prefix = path.dirname(process.execPath);

Is there any reason for assuming the npm runs under the APPDATA path?

Currently, this gives back the wrong prefix on Windows with nvm (node v8.11.1; nvm 1.1.5)

veljkoz commented 6 years ago

Result of #5, as pointed by @doowb ... I will investigate some more tomorrow... thanks

doowb commented 6 years ago

This PR added the APPDATA check and gives an explanation as to when it should be used. It basically says that if someone re-installs npm using npm install npm --global then the node_modules will go into that APPDATA folder instead of process.execPath.

So depending on how you've installed npm (bundled with node, which nvm does, or through npm itself) the path will be different. There might be a way to check this, but I don't know if it will slow it down. You can also set the prefix in .npmrc files in your home directory or a local directory if you need more control over the prefix.

doowb commented 6 years ago

I reopened this for discussion because after thinking about it, I think it would make more sense to take advantage of the logic we already have for checking the .npmrc files. This would then remove the need for checking APPDATA at all. If someone needs to change the path to APPDATA, then that can be done using PREFIX or .npmrc.

I think the most likely place to find the node_modules will be where npm is executing so that would be process.execPath (since this falls back to what npm is doing). Also, since npx was released, this has caused some issues that we might be able to also fix.

veljkoz commented 6 years ago

If in doubt, maybe it would be a good idea to compare with the results of module.paths and return the first match?

veljkoz commented 6 years ago

I just tried to install older npm via nvm, then update it using npm install -g npm and still it installed it next to the nodejs (C:\Program Files\nodejs), which is not surprising given that the:

npm config get prefix -g outputs

C:\Program Files\nodejs

I'm now really unsure for the github docs on npm pointed out in #5 -> here... how does one install the global modules under the APPDATA? I can't seem to replicate this... (without explicit tampering of global env, of course).

jonschlinkert commented 6 years ago

Any thoughts on what our best option is? I can try to implement a fix if we have consensus on what should be done.

jonschlinkert commented 5 years ago

@veljkoz @doowb friendly reminder... I'm working on these now if you have any suggestions.

doowb commented 5 years ago

After reading through this again and considering the referenced docs have moved, it seems like using process.execPath might be the best bet or looking to see if npm is doing anything differently now. I'm wondering if the process.execpath didn't work before because whatever was using global-prefix was executed through node directly instead of through npm which might show a different path.

I think I've looked around to figure out if nvm sets any environment variables to indicate where npm is installed but I didn't find anything at the time.

If you figure something out and want me to do some testing on windows, I have a laptop I can use.

veljkoz commented 5 years ago

Agreed - I'd vote to revert the https://github.com/jonschlinkert/global-prefix/pull/5/commits/d8d1afb8d3e1a9c1c2b5b29a3d212ccb1244078e

The other approach would be to use modules.paths[0], but I have a feeling like it wouldn't be "future-proof"...

jonschlinkert commented 5 years ago

Agreed - I'd vote to revert the d8d1afb

Perhaps we can just do fs.existsSync() on the path returned from path.dirname(process.execPath) and return it if it exists. Otherwise, continue with the other logic?

wuwb commented 5 years ago

not work with system link which nvm made to link the default node install path and the specific version node path.