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

Wrong Node.js version displayed when using nvm.fish? #303

Closed
lydell opened this issue May 15, 2022 · 12 comments
Closed

Wrong Node.js version displayed when using nvm.fish? #303

lydell opened this issue May 15, 2022 · 12 comments
Labels
🐛 bug Something isn't working

Comments

@lydell
Copy link

lydell commented May 15, 2022

Describe the bug

The prompt seems to always print the “system” Node.js version, not the one currently enabled via nvm.fish.

Sorry if this isn’t a problem with tide! I’ve looked around in the issue trackers of both tide and nvm.fish for a long time and I’m very confused at this point! Seems to have been much back and forth and some collaboration in this area.

Steps to reproduce

  1. I have installed Node.js 16.14.2 via brew.
  2. nvm install 18
  3. node -v now says v18.1.0
  4. But my prompt still says 16.14.2

Screenshots

image

Environment

Output of tide bug-report:

fish version: 3.4.1
tide version: 5.3.0
term: xterm-256color
os: macOS Monterey
terminal emulator: iTerm2
fish startup: 20.06 millis
fisher plugins: jorgebucaran/fisher IlanCosman/tide@v5 jorgebucaran/nvm.fish
@lydell lydell added the 🐛 bug Something isn't working label May 15, 2022
@IlanCosman
Copy link
Owner

Oh, huh, yah that makes sense because the node --version is getting run in a background shell where the system node is still present 😅 I'll have to think about what to do 🤔

@ovikk13
Copy link

ovikk13 commented May 19, 2022

This is also relevant for phpbrew! phpbrew use only updates the $PATH variable of the current shell:)

@lydell
Copy link
Author

lydell commented May 28, 2022

I played around with editing the node item locally:

function _tide_item_node
    # test -e package.json && _tide_print_item node $tide_node_icon' ' (node --version | string trim --chars=v)
    _tide_print_item node $tide_node_icon' ' (nvm current | string trim --chars=v)
end
  • nvm current prints the correct version, and it’s fast.
  • I removed test -e package.json because I prefer to always show the Node.js version.
  • The above code obviously does not work for everyone :) Just sharing

@AdamMomen
Copy link

AdamMomen commented May 28, 2022 via email

@lydell
Copy link
Author

lydell commented Jun 14, 2022

I found an edge case with my solution above.

If I do echo 16 > .node-version, the prompt will say v16.x.x, but in reality I’m still on whatever Node.js version I was on before until I run nvm use.

Not the end of the world, but a consequence of the prompt being run in a background shell I guess – since it’s a different shell it can never know for sure.

@lydell
Copy link
Author

lydell commented Jun 18, 2022

Another edge case: Let’s say you have a .node-version with, say, 18 in it, because you want to use Node.js 18 in that project. Then you’re like “let me just try this one little thing with Node.js 16 real quick” and run nvm use 16 temporarily to try something out before switching back. During that time, the prompt will still say 18 since it is in a “different world” not knowing about that temporary switch.

Still, my solution gives the the correct result maybe 90 % of the time so it’s pretty good.

@jorgebucaran
Copy link
Contributor

Is there anything I could do here or over at nvm.fish to help with this? @lydell @IlanCosman 👋

@IlanCosman
Copy link
Owner

@jorgebucaran I don't think so, unless you want to set the node version in subshells, which doesn't seem appropriate. This is just a fundamental limitation of a sketchy async implementation like Tide has.

@lydell
Copy link
Author

lydell commented Aug 6, 2022

I looked around in the code to see if I could understand how this async stuff works. Is it this line?

https://github.com/IlanCosman/tide/blob/v5.4.0/functions/fish_prompt.fish#L40-L41

fish -c '...' & + disown? If so, I don’t understand why PATH isn’t passed along there. I tried to play around with it:

# Switch to Node.js 18 with nvm.fish:
❯ node -v
v16.15.1
❯ echo 18 > .node-version
❯ nvm use
Now using Node v18.3.0 (npm 8.11.0) ~/.local/share/nvm/v18.3.0/bin/node
❯ node -v
v18.3.0

# bash inherits PATH:
❯ bash -c 'node -v'
v18.3.0

# fish does not – why?
❯ fish -c 'node -v'
v16.15.1

# With -N it does?
❯ fish -Nc 'node -v'
v18.3.0

# We can also explicitly pass PATH:
❯ COOL_PATH=$PATH fish -c 'PATH=$COOL_PATH node -v'
v18.3.0
  • Out of curiosity, do you know why fish -c 'node -v' or fish -c 'echo $PATH' does not include the nvm.fish path, while bash -c does?
  • Would it be possible to use fish -Nc or the COOL_PATH trick to solve this issue? In other words, keep the node -v approach and not use nvm current. Edit: -N seems to break everything. Edit2: COOL_PATH seems to work perfectly, including all edge cases I posted above!

@IlanCosman
Copy link
Owner

Omg I can't believe I didn't think of just passing PATH to the subshell 🤦‍♂️ Thanks a ton for pointing that out @lydell 👍

IlanCosman added a commit that referenced this issue Aug 6, 2022
IlanCosman added a commit that referenced this issue Aug 6, 2022
@IlanCosman
Copy link
Owner

Alright, this should be fixed with the latest change on master. Hopefully the change doesn't cause any problems 😅 Will do a release soon.

@jorgebucaran
Copy link
Contributor

Great observation @lydell 💯

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐛 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants