`node_version_prompt` should work without NVM #1946

kigster · 2021-09-13T07:12:17Z

Description

Adding node prompt that does not depend on nvm and will work with other version managers as well.

Motivation and Context

There are now alternative version managers available, such as a much more streamlined volta.sh. It feels like a deja-vu of rvm to rbenv switch, all over again.

In any case, we should be able to show node version regardless of whether you are using NVM or not.

I decided not to add dedicated PREFIX variables for now, but it can be done later.

We still check if nvm prompt returns something first because the declare check is practically free, and if it returns something — we use it. Only if the output of NVM is blank do we use the new function to grab the version of NodeJS.

There is a caveat — if node is installed with the OS, eg /usr/bin/node the new function will now pick up the version of that "system" node and show it. Therefore "system" node version will now be visible in the prompt of those who added node component to their prompt. Personally, I believe this is the correct behavior, because why should we hide the system node version if that's what's available and in the PATH? We shouldn't. In fact, I think it's rather confusing that previously we wouldn't show the system node version at all.

How Has This Been Tested?

Tested locally on OS-X/bash:

* with/without NVM
* with/without VOLTA
* with/without system node

Screenshots (if appropriate):

Powerline Multiline with VOLTA version manager and no NVM:

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
If my change requires a change to the documentation, I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
I have added tests to cover my changes, and all the new and existing tests pass.

Adding `node` prompt that does not depend on `nvm` and will work with other version managers as well. There are now alternative version managers available, such as a much more streamlined [volta.sh](https://volta.sh). It feels like a deja-vu of `rvm` to `rbenv` switch, all over again. Regardless, we should be able to show the current `node` version whether you are using NVM, VOLTA or a hot potato. I decided not to add dedicated PREFIX variables for now, but it can be done later. We still check if `nvm` prompt returns something first because the `declare` check is practically free, and if it returns something — we use it. Only if the output of NVM is blank do we use the new function to grab the version of NodeJS. There is a caveat — if `node` is installed with the OS, eg `/usr/bin/node` the new function will now pick up the version of that "system" node and show it. Therefore "system" node version will now be visible in the prompt of those who added `node` component to their prompt. Personally, I believe this is the correct behavior, because why should we hide the system node version if that's what's available and in the PATH? We shouldn't. In fact, I think it's rather confusing that previously we wouldn't show the system node version at all. Tested locally on OS-X/bash: * with/without NVM * with/without VOLTA * with/without system node

NoahGorny

Hi @kigster, great addition!
It seems like the lint stage is failing, so you probably need to fix that.
Other than that- I would be delighted if you could add tests for this. You can add a mock nvm/node similar to whats being done in base.theme.svn.bats
what do you think?

themes/base.theme.bash

gaelicWizard · 2022-02-24T00:06:10Z

@kigster, I opened kigster#3 which fixes the linting failure and alsö addresses @NoahGorny's suggestion. If you accept that PR, then it will automatically update this PR, which I then think is ready to merge.

kigster · 2022-02-24T06:31:34Z

@gaelicWizard Thank you so much! My apologies, I started a new job and this fell off my radar. Glad you were able to finish it!

gaelicWizard

The test failure with upower should be fixed in #2061. The linting error just needs a new line, suggested in my comment

themes/base.theme.bash

Co-authored-by: John D Pell <[email protected]>

gaelicWizard · 2022-02-25T01:03:11Z

@NoahGorny, this one seems good to go, except for tests. I'm about to overhaul the BATS infra so I'm willing to add any missing coverage at that point unless @kigster has some tests up his sleeve.

kigster · 2022-02-25T01:45:08Z

I don't have anything at the moment, like I said — new job :)

kigster · 2022-03-24T23:19:58Z

Conflicts resolved, any chance to get this merged?

kigster · 2022-03-24T23:21:33Z

Also, locally with shellcheck enabled, I don't have any issues around the lines I've changed. If the linter checks the entire file, then I am not really sure it's appropriate for me to be fixing everyone else's lint issues?

kigster · 2022-03-24T23:22:33Z

The docs CI error is the following:

ImportError: cannot import name 'environmentfilter' from 'jinja2' 
(/opt/hostedtoolcache/Python/3.8.12/x64/lib/python3.8/site-packages/jinja2/__init__.py)

I am certain this change has nothing to do with this failure.

cornfeedhobo · 2022-04-01T17:43:43Z

themes/base.theme.bash

+	local node_version
+	if _command_exists node; then
+		node_version="$(node --version 2> /dev/null)"
+		if [[ -n ${node_version} ]]; then


This check would give a false positive when using version management shims that are common with tools like asdf

Could you clarify? What do you mean by a false positive?

if node --version returns a non-empty output, doesn't that mean that this is the current node version? If so, why not show it in the prompt?

seefood

seems good, but needs a new merge from master because there are conflicts. please resolve the conflicts and I'll merge it.

kigster · 2024-11-19T18:19:29Z

Sounds good. I noticed the same problem with ruby.

Should I submit a separate PR for Ruby or include it here?

seefood · 2024-11-19T19:44:35Z

Sounds good. I noticed the same problem with ruby.

Should I submit a separate PR for Ruby or include it here?

new issue = new PR. thanks!

kigster force-pushed the kig/nodejs-version branch 2 times, most recently from 5a94e66 to c612a65 Compare September 13, 2021 07:26

kigster force-pushed the kig/nodejs-version branch from c612a65 to b935ddd Compare September 13, 2021 07:27

NoahGorny requested changes Sep 15, 2021

View reviewed changes

themes/base.theme.bash Outdated Show resolved Hide resolved

lib/theme: use _command_exists()

1a96dd1

Merge pull request #3 from gaelicWizard/theme/nodejs-version

4125123

gaelicWizard requested changes Feb 24, 2022

View reviewed changes

themes/base.theme.bash Outdated Show resolved Hide resolved

Update themes/base.theme.bash

f7c3256

Co-authored-by: John D Pell <[email protected]>

kigster added 2 commits March 24, 2022 14:49

Fix node and ruby colors

78d19e7

Merge branch 'master' into kig/nodejs-version

289a9d9

cornfeedhobo reviewed Apr 1, 2022

View reviewed changes

seefood approved these changes Nov 13, 2024

View reviewed changes

seefood self-assigned this Nov 13, 2024

seefood added seems abandoned rattle the cage, and close if nobody wants to keep it going waiting-for-response labels Nov 13, 2024

seefood marked this pull request as draft November 19, 2024 19:47

seefood removed seems abandoned rattle the cage, and close if nobody wants to keep it going waiting-for-response labels Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`node_version_prompt` should work without NVM #1946

`node_version_prompt` should work without NVM #1946

kigster commented Sep 13, 2021 •

edited

Loading

NoahGorny left a comment

gaelicWizard commented Feb 24, 2022

kigster commented Feb 24, 2022 •

edited

Loading

gaelicWizard left a comment

gaelicWizard commented Feb 25, 2022 •

edited

Loading

kigster commented Feb 25, 2022

kigster commented Mar 24, 2022

kigster commented Mar 24, 2022

kigster commented Mar 24, 2022 •

edited

Loading

cornfeedhobo Apr 1, 2022

kigster May 5, 2022

seefood left a comment

kigster commented Nov 19, 2024

seefood commented Nov 19, 2024

node_version_prompt should work without NVM #1946

Are you sure you want to change the base?

node_version_prompt should work without NVM #1946

Conversation

kigster commented Sep 13, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

NoahGorny left a comment

Choose a reason for hiding this comment

gaelicWizard commented Feb 24, 2022

kigster commented Feb 24, 2022 • edited Loading

gaelicWizard left a comment

Choose a reason for hiding this comment

gaelicWizard commented Feb 25, 2022 • edited Loading

kigster commented Feb 25, 2022

kigster commented Mar 24, 2022

kigster commented Mar 24, 2022

kigster commented Mar 24, 2022 • edited Loading

cornfeedhobo Apr 1, 2022

Choose a reason for hiding this comment

kigster May 5, 2022

Choose a reason for hiding this comment

seefood left a comment

Choose a reason for hiding this comment

kigster commented Nov 19, 2024

seefood commented Nov 19, 2024

`node_version_prompt` should work without NVM #1946

`node_version_prompt` should work without NVM #1946

kigster commented Sep 13, 2021 •

edited

Loading

kigster commented Feb 24, 2022 •

edited

Loading

gaelicWizard commented Feb 25, 2022 •

edited

Loading

kigster commented Mar 24, 2022 •

edited

Loading