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

Discussion to improve reliability on Windows #21

Open
veljkoz opened this issue Jun 19, 2018 · 11 comments
Open

Discussion to improve reliability on Windows #21

veljkoz opened this issue Jun 19, 2018 · 11 comments

Comments

@veljkoz
Copy link

veljkoz commented Jun 19, 2018

This line:

prefix = process.env.APPDATA

//...
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
Copy link
Author

veljkoz commented Jun 19, 2018

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

@veljkoz veljkoz closed this as completed Jun 19, 2018
@doowb
Copy link
Collaborator

doowb commented Jun 19, 2018

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 doowb reopened this Jun 19, 2018
@doowb
Copy link
Collaborator

doowb commented Jun 19, 2018

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.

@doowb doowb changed the title Doesn't work well with nvm for Windows Discussion to improve reliability on Windows Jun 19, 2018
@veljkoz
Copy link
Author

veljkoz commented Jun 19, 2018

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

@veljkoz
Copy link
Author

veljkoz commented Jun 20, 2018

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
Copy link
Owner

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
Copy link
Owner

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

@doowb
Copy link
Collaborator

doowb commented Dec 15, 2018

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
Copy link
Author

veljkoz commented Dec 16, 2018

Agreed - I'd vote to revert the d8d1afb

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

@jonschlinkert
Copy link
Owner

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
Copy link

wuwb commented Oct 29, 2019

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

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

No branches or pull requests

4 participants