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

show only versions newer than NVM_MIN if set #3277

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

ryenus
Copy link
Contributor

@ryenus ryenus commented Jan 29, 2024

This is to fix #3250.

@ryenus

This comment was marked as outdated.

@ryenus ryenus force-pushed the minver branch 2 times, most recently from a81e16a to b10554a Compare January 29, 2024 15:22
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.

I like that it's done in awk :-) it'd be great to benchmark the difference to make sure this isn't slowing anything down meaningfully.

this will need tests, and also it'd be good to add a --min option to all the things that talk to the mirror (nvm ls-remote, nvm install, nvm version-remote) and then maybe nvm ls for good measure (altho that can be a follow-on if you like)

@ljharb ljharb added installing node Issues with installing node/io.js versions. feature requests I want a new feature in nvm! labels Jan 29, 2024
@ryenus
Copy link
Contributor Author

ryenus commented Jan 29, 2024

Performance-wise there's not really much change or overhead, you can trust that awk is fast :-) Sometimes it might even run faster, esp. when many older versions are skipped.

Meanwhile the other sibling functions and even the tests are something I'm not really familiar with, may you can chime in here? @ljharb

@ljharb
Copy link
Member

ljharb commented Jan 29, 2024

Sure - for the tests, there's existing tests for nvm ls-remote that employ mocks for the remote data, and a test that uses the env var, and asserts that it matches the mock minus the first N lines (since older versions aren't going to change, that number won't break over time), that would be sufficient. Similar approaches will work for the others.

For adding an option, if you grep for "ls-remote" you'll find the chunk of code where top-level commands are defined, and there's a bunch of existing examples that process the arguments.

@ljharb ljharb force-pushed the master branch 2 times, most recently from c6cfc3a to c20db2a Compare June 10, 2024 18:13
@ryenus ryenus force-pushed the minver branch 2 times, most recently from abde015 to 132b9fb Compare June 29, 2024 05:54
@ryenus
Copy link
Contributor Author

ryenus commented Jun 29, 2024

@ljharb just added a test, however Travis CI seems stuck, any insight?

@ryenus

This comment was marked as resolved.

@ryenus ryenus changed the title show only versions newer than NVM_MIN_VER if set show only versions newer than NVM_MIN if set Jun 29, 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.

It occurs to me that

nvm.sh Outdated Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
test/fast/Unit tests/nvm_print_versions Outdated Show resolved Hide resolved
@ryenus ryenus force-pushed the minver branch 2 times, most recently from 46690fd to daad693 Compare June 30, 2024 02:51
@ryenus

This comment was marked as resolved.

@ryenus
Copy link
Contributor Author

ryenus commented Jul 14, 2024

As we can see from the below screenshot, for v16.15.1, now all of its updates are listed:

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 looks great, I'll play with it locally a bit this week.

nvm.sh Show resolved Hide resolved
nvm.sh Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Jul 27, 2024

When I run nvm ls locally with this PR, I get empty as the first item in the list.

When I run nvm ls-remote --min=4 locally with this PR, all versions are displayed.

@ljharb ljharb marked this pull request as draft July 27, 2024 20:40
@ryenus
Copy link
Contributor Author

ryenus commented Jul 28, 2024

  • When I run nvm ls locally with this PR, I get empty as the first item in the list.

    Interesting, any idea where could this empty come from? Asking because here the change to nvm_print_versions only determines whether to skip certain versions or not, meanwhile all the versions are supplied by the caller.

    May I ask what's the output of nvm ls when run from the master branch? Asking because for me I got identical output:

    image
  • When I run nvm ls-remote --min=4 locally with this PR, all versions are displayed.

    As a comparison, here's what I got with the same command:

    image

    Is it possible that the shell was still running the code from another branch, rather than that of the PR? Maybe check the output of type nvm_print_versions?

@ljharb
Copy link
Member

ljharb commented Jul 29, 2024

ohhh wait, lol i think this is the actual feature i asked for - i have tons of node versions installed, going back a very long way:

$ NVM_MIN=4 nvm ls-remote | wc -l
     711
$ nvm ls-remote --min=4 | wc -l
     711
$ nvm ls-remote | wc -l
     790

in other words, it's filtering some things out, but with the additional logic i requested, it's still showing me a lot of things i didn't expect. 🤔

@ryenus
Copy link
Contributor Author

ryenus commented Jul 29, 2024

ohhh wait, lol i think this is the actual feature i asked for - i have tons of node versions installed, going back a very long way:

in other words, it's filtering some things out, but with the additional logic i requested, it's still showing me a lot of things i didn't expect. 🤔

Ahh, so it's because you have just too many versions installed? Or would you change mind? Hopefully not :P

BTW, typically I just run something like NVM_MIN=18 nvm ls-remote --lts.

@ryenus ryenus marked this pull request as ready for review July 29, 2024 02:33
@ryenus
Copy link
Contributor Author

ryenus commented Jul 30, 2024

@ljharb FYI, just rebased with some minor fix in tests.

@ryenus
Copy link
Contributor Author

ryenus commented Aug 13, 2024

@ljharb may I ask how it is going? anything to note?

@ljharb
Copy link
Member

ljharb commented Aug 13, 2024

No progress yet; I've been traveling. I'm prioritizing a bug fix on nvm before this PR, but it'll be next in line after that.

@ljharb
Copy link
Member

ljharb commented Oct 18, 2024

ok, so i just tried it out again.

if i do nvm ls-remote --min=99, i get all the versions hidden below the lowest one i have installed - v0.4.12 - but i get every version higher. So, for me, it's not really that useful. I assume that on an empty system, or one where all installed node versions are above the min, it actually works as intended.

I'm starting to wonder if perhaps my original concern - about versions being hidden - is better addressed by prepending a message like "Minimum version X set: N versions hidden", and going back to the simpler logic? (i'm super sorry if this means you're doing work to undo the work i already asked you to do :-/ unfortunately sometimes it takes time and first-hand experience to really get a sense of the DX/UX of a thing)

@ryenus
Copy link
Contributor Author

ryenus commented Oct 19, 2024

@ljharb, although in some cases it's somehow possible to have a broad range of node versions installed, but I'm sure not everyone would need to do that. I believe most people only need (a) several (if not a few), and (b) most recent, (c) typically LTS versions, installed for the project(s) they work on. Like what's shown in a previous comment:

And this is sort of the next step for the previous pr #2827, that one has improved the performance of nvm ls-remote, by reducing the time needed for a complete nvm ls-remote from about 20s down to about 2s. This pr aims to also improve the ergonomics, by showing just what's really needed, like 798 (before) vs 38 (after) as shown below, nevertheless the time taken is also reduced as a side effect:

UnfilteredFiltered
# time nvm ls-remote --min=v0 | wc -l
798

real	0m1.253s
user	0m0.246s
sys	0m0.322s
# time nvm ls-remote --min=18 --lts | wc -l
38

real	0m0.545s
user	0m0.154s
sys	0m0.188s

Meanwhile no worries, your initial proposal actually inspired me to think more about why people would need nvm ls-remote, or even nvm: it's because people would want to find newer versions of node delivering new features, patches or whatever improvements that are not available in currently installed versions. In that sense showing new patch versions definitely helps. That works like the program automatically detects a suitable min version based on installed ones, saving the need to figuring out the closest min version to specify, which is actually nice.

@ryenus
Copy link
Contributor Author

ryenus commented Oct 19, 2024

ryenus and others added 16 commits December 13, 2024 09:35
show only versions newer than NVM_MIN_VER if set
since nvm is all about versions, so no need for the explicit suffix.
even if they're older than $NVM_MIN
When omitted, fallback to the environment variable "NVM_MIN" if set.
And the CLI option --min=<version> takes precedence over the environment
variable "NVM_MIN" if both are present.
This is to inherit $NVM_MIN from env when defined, meanwhile avoiding
inline local variable initialization for ksh compatibility.

Co-authored-by: Jordan Harband <[email protected]>
due the use of --min or NVM_MIN, which would show only the versions
higher than the specified min version, and/or the available minor/patch
updates for the installed versions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ls-remote] only list active and/or maintained versions
2 participants