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

add nvm script for use by other shells #582

Closed
wants to merge 2 commits into from

Conversation

danielb2
Copy link
Contributor

  • adds bin/nvm for use as a script by any shell
  • updates documentation on how to use script

@danielb2 danielb2 mentioned this pull request Nov 21, 2014
#!/bin/bash

# For non bash-shells (fish, etc)
# add <prefix>/nvm/bin and <prefix>/nvm/current/bin to PATH
Copy link
Member

Choose a reason for hiding this comment

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

Should this be part of the install script as well?


Where nvm is installed at `~/.nvm`

set -x fish_user_paths $PATH $NVM_DIR/.nvm/current/bin $NVM_DIR/.nvm/bin
Copy link
Member

Choose a reason for hiding this comment

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

$NVM_DIR already contains .nvm - you want $NVM_DIR/current/bin $NVM_DIR/bin

Also, please note, I'll soon be switching the default behavior of nvm to NOT create the "current" symlink.

@ljharb
Copy link
Member

ljharb commented Nov 21, 2014

nvm is meant to be specific to a single shell. The "current" symlink is problematic because of this, and will be removed in a future version. Similarly, a binary that doesn't run in the context of the current shell might be problematic as well.

@danielb2
Copy link
Contributor Author

How is the current symlink problematic? Does it interfere with something? I implemented that as unobtrusively as I could think of the first time around (#447), and I don't see how it can be an issue.

Also, considering the relative ease at which nvm can be made to support ALL shells, this seems like a straight forward thing to implement.

If at some point it actually presents a real problem, I can see the justification in removing it, but not just for the sake of what "might be".

@ljharb
Copy link
Member

ljharb commented Nov 21, 2014

If in one shell I have v0.11 and in another I have v0.10, which one is current?

The correct answer is "neither" - the concept of a "current" version only exists in a single shell, and is totally invalid beyond that, including in the filesystem.

@danielb2
Copy link
Contributor Author

When you mean one shell, and another shell, I assume you mean instances of each shells as opposed to bash vs fish for instance.

I thought quite awhile before settling on the name current. I also considered default. But regardless it sounds like your actual concern is not with the implementation itself but with the actual naming of the symlink.

While the implementation is restricted to not include the scenario you describe, I'm guessing you're still addressing a very large amount of use-cases (certainly mine) out there. If there's a better way of implementing it then surely you can consider it later rather than abandoning this one simply because it doesn't cover all use-cases ?

@ljharb
Copy link
Member

ljharb commented Nov 21, 2014

The original use case, as I recall, for the current symlink was to support IDEs. However, as it's since become clear to me, nvm isn't meant to switch the version of node that everything on the system uses - only the PATH for the current shell instance (and all subshells), which thus causes the desired version of node to be found.

The naming aside, the entire concept of the symlink doesn't make sense based on this interpretation of what nvm is meant to be for.

However, I really like the idea of your approach as a way to begin solving issues with other shells (like fish), and perhaps as a way to remove the need for most of nvm to be portable across shells by using a portable loader and invoking the rest of the code in a single consistent shell. I'd love to find a way to bring that in such that it doesn't rely on the state of the filesystem; only the $PATH of the current shell environment.

@nvcexploder
Copy link

What about folks who are using nvm to change system node versions for regression testing on libraries that they maintain?

@ljharb
Copy link
Member

ljharb commented Nov 21, 2014

@nvcexploder You can do any regression testing you need within a shell environment. All node project tests on travis-ci are run with nvm and they work just fine.

@danielb2
Copy link
Contributor Author

Original use-case was #430 which is also mine, but basically it was just to support fish-shell. #467 had an IDE use-case but the implementation that ended up being used was mine. Both our use-cases were solved by this.

Having a preconceived idea of what nvm is for shouldn't limit what we use it for if such usage is possible.

The naming aside, the entire concept of the symlink doesn't make sense based on this interpretation of what nvm is meant to be for

I find this statement surprising as while nvm isn't using a symlink to select the default version of node, it creates a file called default with the version number. This is really the same behavior. I just found it easier to deal with a symlink than it was to read a file and create the PATH based on that.

Speaking of which, one issue I've found with nvm while using bash and modifying the path is that it has actually broken the path for me several times because it keeps appending to it making it eventually huge. Using the symlink in fish I've had zero issues.

But yes, the portability to other shells is far easier to deal with if nvm is invoked as a separate script instead.

I could see an API such as:

nvm which 0.10 that returns a path. Similiarly nvm which default for instance.

Then other shells would just need a wrapper per shell to deal with setting the path.

@ljharb
Copy link
Member

ljharb commented Nov 21, 2014

"default" is just an alias. Saying that "x" is default doesn't mean in any way that a given shell is running "x", just that it /starts/ as "x".

I would absolutely be in favor of a PR that adds nvm which X and returns a path to the node binary.

Let's get that in first, and then it might be much simpler to solve the fish/other shell/IDE issue with that. Thoughts?

@danielb2
Copy link
Contributor Author

"default" is just an alias. Saying that "x" is default doesn't mean in any way that a given shell is running "x", just that it /starts/ as "x".

Yeah, that was really all the functionality I was looking for fish since it solved my whole use-case.

Let's get that in first

I can do that

@danielb2 danielb2 closed this Nov 21, 2014
@danielb2
Copy link
Contributor Author

actually, going to reopen this and ask still that you consider this immediately as either way, this script would be useful both with the current symlink, and after nvm which is implemented.

@danielb2 danielb2 reopened this Nov 21, 2014
@@ -0,0 +1,8 @@
#!/bin/bash

export NVM_SYMLINK_CURRENT=true
Copy link
Member

Choose a reason for hiding this comment

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

Must this env var be forced to true, for this script to work, rather than just using the environment setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but the script uses about 90% of it's value without it. Basically you'd be forced to add something to your environmental settings which can just as easily be done here saving the work.

@danielb2
Copy link
Contributor Author

Here's nvm which: #583

@danielb2
Copy link
Contributor Author

closing for now. I think all it will actually need to do is to give the results of nvm which and as such the symlink isn't needed (as you suspected).

And I'll probably just make something to source for fish and include it separately to source.

@danielb2 danielb2 closed this Nov 22, 2014
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.

3 participants