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

Preserve the order of PATH locations when changing node versions #1316

Merged
merged 1 commit into from
Sep 20, 2017
Merged

Preserve the order of PATH locations when changing node versions #1316

merged 1 commit into from
Sep 20, 2017

Conversation

zeorin
Copy link
Contributor

@zeorin zeorin commented Nov 21, 2016

nvm may not be the only utility managing the PATH. The order of locations in the PATH matters, as executables in locations in the beginning of PATH will be shadowing executables that are in later locations.

An example of this is when the output of $(npm bin) is prepended to the PATH. If nvm is used to change the node version after that, it should not change the order of path locations.

It's up to the user to change the PATH in a way that they prefer, by loading utilities in the correct order.

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.

I don't necessarily agree with the claim that the user should be solely managing their PATH - by using nvm, the user is explicitly ceding control of their PATH to it.

nvm.sh Outdated
nvm_echo "${3-}${2-}"
# if the initial path doesn’t contain an nvm path, prepend the supplementary
# path
elif [ "$(expr "${1-}" : ".*${NVM_DIR}/[^:]*${2-}.*")" = 0 ] || \
Copy link
Member

Choose a reason for hiding this comment

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

I've tried to avoid using expr in nvm because of https://github.com/koalaman/shellcheck/wiki/SC2003. Can this be written differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From that link:

sh doesn't have a great replacement for the : operator (regex match). ShellCheck tries not to warn when using expr with :

I'm not aware of another way to do this and still keep POSIX compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

Could it use awk or sed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about using grep?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, any posix tool is great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 😁

@zeorin
Copy link
Contributor Author

zeorin commented Nov 21, 2016

I'm not saying that the user is solely managing their path. Simply that it's up to the user to load their PATH-adapting utilities in an order that makes sense for them.

In the current scenario, nvm is actively hostile to any other PATH-changing utility or script.

nvm's core purpose is not to manage the PATH. Managing the PATH is how nvm achieves its ends, but it's not its raison d'être. It's not a PATH manager. It's a Node manager. The PATH management is incidental.

Whilst I accept, as a user of nvm, that it can't manage my node versions and still provide a good user experience without changing PATH, I find it a stretch to say that a user is explicitly ceding control of their PATH completely to NVM. I'd say it's more implicit, if the user even has any idea at all of what's happening in the first place (the amount of *nix noobs is growing—nothing wrong with that, but what you're saying may not be obvious to them).

In the current scenario:

  • When NVM is sourced, or does almost anything, it's at the front of the PATH and dominates it.
    • Drawbacks: not sensitive to the other, non-nvm needs the PATH must serve
    • Benefits: 100% certain that the current version of node's global cli executables shadow everything else (though that's not always what we want—it's certainly not what I want: I only want it to shadow the system node's global cli executables)

In my fix:

  • When NVM is sourced, or does almost anything, it's at the front of the PATH the 1st time and keeps its place the rest of the time.
    • Drawbacks: something that is relying on NVM clobbering the PATH order may no longer work (honestly, what would be counting on clobbering behaviour?); a user might find a surprising behaviour once and have to think for 5 minutes about the order of their PATH
    • Benefits: NVM can play nice with other PATH-managing utilities. It can be integrated with more tools and workflows because it's not hostile to them by default. It's sensitive, considerate. It's more useful.

In short, my argument is that the PATH doesn't serve only NVM. It serves other needs, too. Always has, always will. For NVM to assume that its needs and demands on the PATH trump the rest of the system's needs, always, no-questions-asked, no-prisoners-taken, is hostile to other tools. Tools that the user has chosen to run. I.e. that assumption is ultimately hostile to the user.

Unix has always been about giving people the freedom to do stupid things. Because without it you don't have the freedom to do clever things.

@ljharb
Copy link
Member

ljharb commented Jun 13, 2017

@zeorin let's give this one another shot - can you rebase it on latest master, and/or check the "allow edits" checkbox on the right hand column?

@ljharb ljharb added bugs Oh no, something's broken :-( feature requests I want a new feature in nvm! labels Jun 13, 2017
@zeorin zeorin changed the title Don't mess with the order of the path locations when changing versions Preserve the order of PATH locations when changing node versions Sep 7, 2017
@zeorin
Copy link
Contributor Author

zeorin commented Sep 9, 2017

@ljharb I rebased the pull request branch on master.

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.

The more I try this out locally the more I'm convinced this is an excellent change.

Thanks for bearing with me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugs Oh no, something's broken :-( feature requests I want a new feature in nvm!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants