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

303 nvm in fish #579

Closed
wants to merge 7 commits into from
Closed

303 nvm in fish #579

wants to merge 7 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 15, 2014

Hi,

Related to #303 , I use fish myself and love that I can run npm install -g <...> with nvm --> fish nvm was needed.

This is simply a wrapper around nvm for fish. I have commented my commits as much as possible to help understand my thought process. I hope others with fish can test this (tests in vagrant worked for me).
A wrapper allows nvm to change below without (hopefully) affecting functionality in fish.

I'm open to critical feedback or improvement suggestions.

Cheers

LoveIsGrief added 4 commits November 15, 2014 22:26
It will print all paths that have been changed by nvm use. These paths are exported for all later nvm commands and need to be applied to the environment of other shells (in this case fish), therefore they are printed with the option.
nvm.sh isn't executable so this file was introduced. It dereferences symlinks for e.g `ln -s ~/.nvm/nvm ~/bin/nvm` will now work.
This is a wrapper around nvm. It's main function is to check and promote environment variables changes from nvm to fish.

A little more is done to setup the environment in case that hasn't been done yet:

* install a stable node version if it doesn't exist
* set the default to stable alias if that hasn't been done yet

Due to those actions, the first execution of `nvm` might take some time.
Fish has to be configured to use nvm.
Basically 2 actions have to be taken (like with bash and others):
1. Introduce the nvm function
2. Execute the nvm function once the shell is loaded to have access to nodejs
   The first session with fish might take a while to start,
   if nvm doesn't have its default env (stable nodejs + default alias)
@ljharb
Copy link
Member

ljharb commented Nov 16, 2014

Per #303, https://github.com/Alex7Kom/nvm-fish works well in fish. This PR seems like it's way too large to add just to support one shell, especially an uncommon one such as fish. I'll keep it open for awhile while I evaluate it, but I'm not inclined to merge it.

If I was inclined, it would need to be accompanied by test suite changes so that the full suite of tests were run in travis-ci in the fish shell.

@@ -1,5 +1,15 @@
#!/bin/bash

# Get this script after dereferincing all symlinks
# !! Doesn't check for circular symlinks !!
Copy link
Member

Choose a reason for hiding this comment

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

This is a pretty big caveat. Can it be avoided? Why is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Oops, sorry. That's for the vagrant PR #578 . It isn't used in this one. I'll remove it.

LoveIsGrief added 3 commits November 16, 2014 15:28
The DIR variable is used in another branch (vagrant) and doesn't belong here
…bles.' work again

/bin/sh doesn't like `while [[ ... ]]`. It much rather prefers `while [ .. ]`

Also minor spacing changes
…k again

Slight problem with the output. The test expected an alias to be returned, but an empty string was returned.
@danielb2
Copy link
Contributor

Related: #582

My PR is not shell specific, but will work with fish or any other shell

@ljharb
Copy link
Member

ljharb commented Dec 21, 2014

Now that nvm which is merged, can this be simplified and freshly rebased onto master, so we can continue the discussion?

@ljharb ljharb added the needs followup We need some info or action from whoever filed this issue/PR. label Dec 21, 2014
@ghost
Copy link
Author

ghost commented Jan 17, 2015

I'm not sure what could be simplified. nvm which only prints out the path to the node binary. The changes I made print out more paths:

@@ -858,6 +873,12 @@ nvm() {
       if [ "$NVM_SYMLINK_CURRENT" = true ]; then
         rm -f "$NVM_DIR/current" && ln -s "$NVM_VERSION_DIR" "$NVM_DIR/current"
       fi
+      if [ $PRINT_PATHS ]; then
+        echo PATH=$PATH
+        echo NODE_PATH=$NODE_PATH
+        echo NVM_PATH=$NVM_PATH
+        echo NVM_BIN=$NVM_BIN
+      fi
       echo "Now using node $VERSION"
     ;;
     "run" )

@ljharb
Copy link
Member

ljharb commented Jan 17, 2015

A small, separate PR for --print-paths alone would be useful, since it's the main nvm/dereferencing symlinks change that I'm most concerned about.

I'd wait a couple of days, though, as my imminent io.js changes will likely conflict.

This was referenced Feb 7, 2015
@ljharb
Copy link
Member

ljharb commented Jun 26, 2016

Closing this for now.

@ljharb ljharb closed this Jun 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs followup We need some info or action from whoever filed this issue/PR. shell: fish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants