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

lydell commented 2 years ago

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

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

Screenshots

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

IlanCosman commented 2 years ago

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 commented 2 years ago

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

lydell commented 2 years ago

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 commented 2 years ago

Thanks a million, Simon, it worked!

On Sat, 28 May 2022 at 12:55, Simon Lydell @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/IlanCosman/tide/issues/303#issuecomment-1140225298, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANKWY7W7WKSBJS3UTN6HUGDVMHURNANCNFSM5V7UGMEQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

lydell commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

IlanCosman commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Great observation @lydell 💯

IlanCosman / tide