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] install: Ignore npm command under $NVM_DIR when checking for global modules #2348

Merged
merged 1 commit into from
Dec 1, 2020
Merged

[Fix] install: Ignore npm command under $NVM_DIR when checking for global modules #2348

merged 1 commit into from
Dec 1, 2020

Conversation

dmolesUC
Copy link
Contributor

@dmolesUC dmolesUC commented Nov 17, 2020

It's not an especially clever check — just a substring match — but should cover a typical situation.

Fixes #2339.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This will need a regression test.

install.sh Outdated Show resolved Hide resolved
@dmolesUC
Copy link
Contributor Author

dmolesUC commented Nov 17, 2020

Re regression tests — I may have to leave that to someone better at sh than I am. I did some manual testing and found that:

  1. if nvm is not already installed, the warning appears if and only if npm is on the path and it has global modules
  2. if nvm is already installed and there is an nvm-installed Node, the warning does not appear:
    a. even if there is another npm on the path with global modules
    b. and even if that npm is ahead of the nvm-installed version on the path
  3. if nvm is already installed, but there is no nvm-installed Node, the warning does appear (if npm is on the path and it has global modules)

I think 2a and 2b are probably still the correct behavior in terms of #2339? We can assume in that case the user already has some idea what nvm is and the difference between system and nvm-managed Node.

But I can see an argument the other way as well — people may still want to know that those modules are hanging around, esp. if their binaries might end up ahead of nvm-installed binaries in the path. It's probably more trouble than it's worth to exhaustively search $PATH for possible npm installations, but we might need to move the call to nvm_check_global_modules ahead of where we source nvm.sh.


Note: I wasn't sure about 2b, till I realized the second time I thought I was trying 2b I was actually trying 3.

@dmolesUC
Copy link
Contributor Author

dmolesUC commented Nov 17, 2020

Looks like the relevant unit test is failing in CI. I'll take a look at that.

@dmolesUC
Copy link
Contributor Author

Having trouble replicating the failure locally — I'll see if I can get it failing in a container.

@dmolesUC
Copy link
Contributor Author

OK, still not sure why I couldn't get it to fail on my Mac — something different about my shell setup vs. the Travis environment, such that inside the test NVM_DIR wasn't set.

I've tweaked the test a little so most of the cases are with NVM_DIR= (blank) so that the nvm-installed npm appears to be a system npm, and added a test where NVM_DIR is set, to detect that.

I suppose the alternative would be to have Travis install npm with apt, and try nvm use system, but that seems like it might be overkill.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2020

I think that would indeed be overkill :-) a mock should be sufficient.

@dmolesUC
Copy link
Contributor Author

OK! Have a look at the test and see what you think.

@ljharb ljharb merged commit d4eba35 into nvm-sh:master Dec 1, 2020
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.

install.sh reports modules under ~/.nvm as if they were system modules
2 participants