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

[patch] give a more helpful message when lts alias is mistakenly used #3441

Merged
merged 1 commit into from
Oct 6, 2024

Conversation

r4reetik
Copy link
Contributor

@r4reetik r4reetik commented Oct 5, 2024

Issue

When using the command nvm use lts, users encounter the following error message:

N/A: version "lts" is not yet installed.
You need to run nvm install lts to install and use it.

However, the LTS version is already installed on the system, and the command nvm use --lts works correctly. This inconsistency causes confusion and inconvenience for users who expect nvm use lts to function as intended.

Solution

The issue was identified in the nvm_remote_version function within the nvm.sh script. The function did not correctly handle the lts alias, leading to the error message.

To resolve this, I modified the nvm_remote_version function to include a case for the lts alias. The updated function now correctly maps the lts alias to the latest LTS version.

  • nvm.sh: Added a case for "lts" to the nvm_remote_version() function to retrieve the latest LTS version using nvm_ls_remote.

Impact

This change ensures that the command nvm use lts correctly maps to the latest LTS version, providing a consistent and expected user experience. Users will no longer encounter the error message when the LTS version is already installed on their system.

@ljharb ljharb added the feature requests I want a new feature in nvm! label Oct 5, 2024
@ljharb
Copy link
Member

ljharb commented Oct 5, 2024

lts isn't an alias, by design - nvm use lts isn't supposed to work.

You can nvm use --lts, or nvm use lts/*, or nvm use lts/argon etc.

(even if i wanted to accept this enhancement, it would need thorough tests)

@ljharb ljharb marked this pull request as draft October 5, 2024 04:11
@r4reetik
Copy link
Contributor Author

r4reetik commented Oct 5, 2024

I understand that nvm use lts is not intended for use, but the subsequent error message is misleading, suggesting that nvm install lts should be used instead of nvm install --lts.

If we cannot modify the lts alias without thorough testing, could we at least update the error message to be more accurate in the case of lts?

@ljharb
Copy link
Member

ljharb commented Oct 5, 2024

Yes, in general the error message can be improved; please do update this PR to do so instead.

@r4reetik
Copy link
Contributor Author

r4reetik commented Oct 5, 2024

A change to the nvm_ensure_version_installed function in the nvm.sh script. The change improves the error messaging for users trying to use Node.js LTS versions.

  • nvm.sh: Updated the error message to provide specific instructions for installing and using the LTS version of Node.js when PROVIDED_VERSION is set to 'lts'.

@r4reetik r4reetik marked this pull request as ready for review October 5, 2024 04:49
@r4reetik r4reetik changed the title Fix nvm use lts Command to Correctly Map to Latest LTS Version Fix nvm use lts command to show better error message. Oct 5, 2024
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This is fine, but we'll need a test for it.

@r4reetik r4reetik requested a review from ljharb October 5, 2024 06:16
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM

nvm.sh Outdated Show resolved Hide resolved
@ljharb ljharb removed the feature requests I want a new feature in nvm! label Oct 5, 2024
@ljharb ljharb force-pushed the chore/use-lts branch 2 times, most recently from 9877db5 to 5dc31ac Compare October 6, 2024 01:49
@ljharb ljharb changed the title Fix nvm use lts command to show better error message. [patch] give a more helpful message when lts alias is mistakenly used Oct 6, 2024
@ljharb ljharb merged commit 5dc31ac into nvm-sh:master Oct 6, 2024
170 of 172 checks passed
@Vela427

This comment was marked as spam.

Dr-Bashir

This comment was marked as spam.

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