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 14 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
Copy link
Contributor Author

ryenus commented Jan 29, 2024

For example, running NVM_MIN_VER=18 nvm ls-remote --lts when v14 and v16 are installed:

image

@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.

@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 Show resolved Hide resolved
test/fast/Unit tests/nvm_print_versions Outdated Show resolved Hide resolved
@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 28, 2024

I'm certain that I'm using the code from this PR, yes.

I'm running this on a Mac; if you're not, then perhaps there's some platform-specific difference here?

@ljharb
Copy link
Member

ljharb commented Jul 28, 2024

hm, ok i did just figure out that i had a $NVM_DIR/versions/node/empty dir for some reason, so at least that's a red herring. Let me test again.

@ljharb
Copy link
Member

ljharb commented Jul 29, 2024

ok, testing again, and nvm ls-remote --min=4 (or 20, or whatever) still has no effect for me on my Mac.

What OS are you using exactly?

@ryenus
Copy link
Contributor Author

ryenus commented Jul 29, 2024

ok, testing again, and nvm ls-remote --min=4 (or 20, or whatever) still has no effect for me on my Mac.

What OS are you using exactly?

Actually I'm also on mac :-) How about NVM_MIN=4 nvm ls-remote? Can you also try to enable set -x within 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 and others added 14 commits August 8, 2024 12:10
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.
Co-authored-by: Jordan Harband <ljharb@gmail.com>
This is to inherit $NVM_MIN from env when defined, meanwhile avoiding
inline local variable initialization for ksh compatibility.

Co-authored-by: Jordan Harband <ljharb@gmail.com>
@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.

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