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

Handle installation for 'fish' shell #690

Merged
merged 3 commits into from
Oct 12, 2016
Merged

Handle installation for 'fish' shell #690

merged 3 commits into from
Oct 12, 2016

Conversation

hasit
Copy link
Contributor

@hasit hasit commented Oct 11, 2016

Summary

This PR adds support for handling installation of yarn on fish shell.

fish is a popular shell. Me and many I know use it regularly for its awesome auto-completion features and convenience.

The current problem is that the yarn install script only checks for bash and zsh shells. This was causing ~/.yarn/bin to not be added to the $PATH in fish.

With this commit, the yarn install script first checks if the user's shell is fish. If so, it adds the following line to ~/.config/fish/config.fish.

set -xg PATH ~/.yarn/bin $PATH 

See Startup (Where's .bashrc?) section in official fish documentation for more information.

Test plan

When yarn/scripts/install_latest.sh is run, following happens:

bash-3.2$ ./install-latest.sh
Installing Yarn!
> Downloading tarball...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    92  100    92    0     0    369      0 --:--:-- --:--:-- --:--:--   368
100   595    0   595    0     0    697      0 --:--:-- --:--:-- --:--:--   697
100 3202k  100 3202k    0     0   566k      0  0:00:05  0:00:05 --:--:--  741k
> Extracting to ~/.yarn...
> Adding to $PATH...
> We've added the following to your /Users/hasitmistry/.config/fish/config.fish
> If this isn't the profile of your current shell then please add the following to your correct profile:

export PATH="$HOME/.yarn/bin:$PATH"

> Successfully installed! Please open another terminal where the `yarn` command will now be available.

After successful installation, my ~/.config/fish/config.fish looks like this:

# Go setup
set -xg GOPATH $HOME/go
set -xg GOROOT /usr/local/go
set -xg PATH $GOPATH/bin $PATH
set -xg GO15VENDOREXPERIMENT 1

# Editor
set -xg EDITOR atom

set -xg PATH ~/.yarn/bin $PATH

'fish' is a popular shell. This commit adds support for proper detection and installation of yarn in 'fish' shell. If the detected shell is 'fish', add path to ~/.yarn/bin in $HOME/.config/fish/config.fish file.
@hildjj
Copy link

hildjj commented Oct 11, 2016

Manipulating fish_user_paths seems like a more modern approach.

set -U fish_user_paths ~/.yarn/bin (~/.yarn/bin/yarn global bin) $fish_user_paths

See https://fishshell.com/docs/current/tutorial.html#tut_path

(note: yarn global bin currently outputs color info even if stdout is not a tty, which makes the second bit fail)

@hasit
Copy link
Contributor Author

hasit commented Oct 11, 2016

@hildjj

Yes and no.

While it is supported, I have come across many problems with the set -U ... approach. See fish-shell#527.

set -xg ... approach still remains as the safe way to set $PATH persistently.

@maxnordlund
Copy link
Contributor

Trying to use the PATH as an universal variable isn't supported by fish. However, setting the fish_user_paths works just fine. Note that this should only be run once on installation since universal variables are saved across sessions. This is also why you shouldn't have setup in config.fish if you can avoid it. I use fish_user_paths, and have for a few years now, and haven't had any problems, but I guess your milage may vary.

@hasit
Copy link
Contributor Author

hasit commented Oct 11, 2016

As @ridiculousfish suggests, the set -U fish_user_paths $fish_user_paths ~/.yarn/bin approach is a good one for adding yarn's path globally and persistently. So, @hildjj and @maxnordlund, I agree with you points.

The problem I see here is that the install script is run in bash. If we run the command set -U ..., bash would not recognize it. We will have to set it in ~/config/fish/config.fish file.

Thoughts?

@maxnordlund
Copy link
Contributor

Maybe fish -c 'set -U ...' would do the trick. I.e. run it inside fish.

@hildjj
Copy link

hildjj commented Oct 11, 2016

Note that adding yarn global bin to the path is either waiting on #728 or some sed work.

Set `~/.yarn/bin` in `$fish_user_paths` instead of setting it in `~/.config/fish/config.fish` file.

See [suggestion from fish documentation](fish-shell/fish-shell#527 (comment)) for more reference.
@hasit
Copy link
Contributor Author

hasit commented Oct 11, 2016

@maxnordlund, @hildjj check the new commit. Let me know what you think.

@@ -31,7 +32,11 @@ yarn_link() {
command printf "${SOURCE_STR}"
else
if ! grep -q 'yarn' "$YARN_PROFILE"; then
command printf "$SOURCE_STR" >> "$YARN_PROFILE"
if [[ $YARN_PROFILE == *"fish"* ]]; then
eval $FISH_SOURCE_STR
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's better to just run the command here instead of evaluating it?

command fish -c 'set -U fish_user_paths $fish_user_paths ~/.yarn/bin'

Since it's not appended to a file anymore.

Copy link
Contributor Author

@hasit hasit Oct 11, 2016

Choose a reason for hiding this comment

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

Makes sense. Saves space of storing a command string as well.

@maxnordlund
Copy link
Contributor

I like it, but see my comment above.

@hasit
Copy link
Contributor Author

hasit commented Oct 11, 2016

@maxnordlund, done and works. Thank you 👍

@maxnordlund
Copy link
Contributor

Kein problem, just happy to help 😸

@maxnordlund
Copy link
Contributor

I made #766 for fixing the escape codes, after that the yarn global bin should work.

@hasit
Copy link
Contributor Author

hasit commented Oct 11, 2016

@maxnordlund looks good. We need to hear from the maintainers on this one. Once your PR gets through, I can add the (~/.yarn/bin/yarn global bin) part too.

@cpojer
Copy link
Contributor

cpojer commented Oct 12, 2016

thank you! I think this is reasonable to support.

cc @kittens

@cpojer cpojer merged commit 16b4e3b into yarnpkg:master Oct 12, 2016
@hasit
Copy link
Contributor Author

hasit commented Oct 12, 2016

@cpojer Yay! Thank you.

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.

4 participants