Skip to content

show only versions newer than NVM_MIN if set#3277

Open
ryenus wants to merge 1 commit intonvm-sh:masterfrom
ryenus:minver
Open

show only versions newer than NVM_MIN if set#3277
ryenus wants to merge 1 commit intonvm-sh:masterfrom
ryenus:minver

Conversation

@ryenus
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

ryenus commented Jun 29, 2024

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

@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
Copy Markdown
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

@ryenus ryenus force-pushed the minver branch 2 times, most recently from 46690fd to daad693 Compare June 30, 2024 02:51
@ryenus ryenus force-pushed the minver branch 2 times, most recently from 3ad81e7 to 33e1b3d Compare July 14, 2024 10:53
@ryenus
Copy link
Copy Markdown
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
Copy Markdown
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.

@ljharb
Copy link
Copy Markdown
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
@ljharb
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

@ljharb
Copy link
Copy Markdown
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 ryenus marked this pull request as ready for review July 29, 2024 02:33
@ryenus
Copy link
Copy Markdown
Contributor Author

ryenus commented Jul 30, 2024

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

@ljharb
Copy link
Copy Markdown
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
Copy Markdown
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

This comment was marked as outdated.

@ryenus
Copy link
Copy Markdown
Contributor Author

ryenus commented Mar 30, 2026

Hi, @ljharb, I've updated the code per your most recent feedback and also rebased.

So now nvm ls-remote --min=<ver> strictly lists versions newer than the min version specified. Hopefully we can move forward on this, thank you!

add CLI option --min=<version>, fallback to env var "NVM_MIN" if set.
display a hint when version filtering is in effect

Other minor changes:
- fix: disable color in nvm_print_versions tests
- trim leading space in mock output due to eslint errors
- prefixed versions like v18 also work
- remove function ref duplication
- avoid inline initialization for ksh compatibility

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