nvm-sh / nvm

Node Version Manager - POSIX-compliant bash script to manage multiple active node.js versions
MIT License
79.08k stars 7.92k forks source link

"help" intercepts commands run by exec and run #3126

Open cseitz opened 1 year ago

cseitz commented 1 year ago

Command

# Run node --help with the specified version
nvm exec lts/hydrogen node --help

What happened?

NVM outputs its help command.

What did you expect to happen?

I expected the --help flag to be passed to node

Potential Solution

I understand why the help command takes precedense since its useful for just plopping it at the end of a command to get help on said command. However, this really ruins any situations where one might be using nvm exec as a replacement to node, effectively making it a pain to run anything else's help command.

My primary usage is to use nvm behind-the-scenes as an environment manager, so any time someone specifies "help" in my use-case, it is intended for node, npm, or whatever the command they are running is and not NVM. The user would never be running nvm directly, but rather a custom CLI that passes args through NVM when necessary to run them at the correct version without updating the default NVM version that's in use elsewhere on the user's machine.

Thus, I propose NVM_NO_HELP=1.

...
  # nvm.sh, line 2820
  case $i in
    --) break ;;
    '-h'|'help'|'--help')
    NVM_NO_COLORS=""
+   if [[ "$NVM_NO_HELP" -eq 1 ]]; then
+     break;
+   fi
    for j in "$@"; do
...

This flag, if specified, will instruct NVM to not respond to help requests.

I'm not very skilled with bash so I imagine there's a better solution out there; but the code snippet above works on my local installation of nvm and lets me bypass the help command.

NVM_NO_HELP=1 nvm exec lts/hydrogen node --help
# correctly outputs node v18's help command!

Conclusion

Please let me know if you think this is something that is worth fixing in future versions of NVM. It would certainly help my use-case and allow me to avoid modifying nvm.sh just to escape help commands.

Thanks.

ljharb commented 1 year ago

The solution here is to use -- to tell nvm to stop processing dashed commands, which is a universal shell convention.

ljharb commented 1 year ago

(In the future you may want to hold off on a PR until you're sure the maintainer is interested)

cseitz commented 1 year ago

Oh, thank you! I tried adding -- but apparently I did it in the wrong place thus lead me down this path.

nvm exec lts/hydrogen -- node --help

Good to know this is already implemented. Thanks!

Is this something we should add to the docs?

ljharb commented 1 year ago

Sure, let's update your PR to change the docs instead :-) It should be able to appear anywhere.

cseitz commented 1 year ago
# ❌ - shows NVM help
nvm -- exec lts/hydrogen node --help
# ❌ - error in nvm-exec line 15
nvm exec -- lts/hydrogen node --help
# ✅ - works perfectly
nvm exec lts/hydrogen -- node --help
# ❌ - nodejs: module not found "--help"
nvm exec lts/hydrogen node -- --help

My initial attempt was the 4th one, but this results in Node.js MODULE_NOT_FOUND: cannot find module '--help'

ljharb commented 1 year ago

it certainly has to go after exec lts/hydrogen, but the last one should certainly work - I'd consider that a bug.

shadowspawn commented 1 year ago

For interest, an alternative approach is to have run and exec pass through following arguments (including options). I got used to this convenience with docker:

% docker run ubuntu ls --help
Usage: ls [OPTION]... [FILE]...
List information about the FILEs (the current directory by default).
Sort entries alphabetically if none of -cftuvSUX nor --sort is specified.
...

n follows this pattern:

% n exec hydrogen node --version
v18.16.1
ljharb commented 1 year ago

@shadowspawn i worry that would cause problems if there's nvm-specific options passed to run and exec.

I certainly think that anything that's not used by nvm should be passed through, though.

ljharb commented 1 year ago

Given that nvm exec lts/hydrogen -- node --help works, i think it actually does make sense to not remove a -- inside the executed command - iow, there's no guarantee it's node, and the -- could be meaningful. I'm not sure there's anything to do here except perhaps update the docs?