always show the latest version of nvm #7296
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Copilot
AI
left a comment
Copilot
AI
left a comment
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 suggestions.
Comments skipped due to low confidence (1)
apps/site/components/Downloads/Release/ReleaseCodeBox.tsx:25
- The term 'nvm' should be consistently capitalized as 'NVM' to match the rest of the codebase.
// Docker and nvm support downloading tags/versions by their full release number
|
This isn't working in staging because of CORS - is that expected?
|
|
ahhhh ofc, i didn't think about that. ideally this version would be fetched on the server, then, so it doesn't need to be done on the client. how is that done with the current setup? |
This comment was marked as resolved.
This comment was marked as resolved.
|
@mikeesto amazing, thanks! i've updated the PR (but still need to write tests) |
|
@ljharb apologies, could you add some screenshots of what the changes are? 🙈 |
|
@ovflowd the only visible change will be that the version number in the nvm install URL will be up to date (it's currently out of date). I'm happy to provide screenshots tho, can you help me get the site running locally? (or i suppose one could click the vercel preview and look there?) |
There was a problem hiding this comment.
I appreciate the patch, but all of this feels something that should be done by something like https://raw.githubusercontent.com/nvm-sh/nvm/latest/install.sh
I'm unfortunately against adding vendor-specific code for a single version manager here. Imagine if every single version manager had some curious requirement such as this one 😅
Could there be a latest.nvm.sh/install.sh?
Ah, thanks for the explanation No screenshots needed 🫡 |
|
I don't think "a version number" is all too vendor-specific? nvm intentionally doesn't provide a "latest" URL because that would allow people to update to an nvm version that may have breaking changes, which would be dangerous - akin to putting The alternative to a PR like this is automation that opens a PR to change the hardcoded version number every time nvm does a release, which will mean that nvm is out of date until a human reviews, merges, and deploys that PR. |
That's precisely why the latest/install.sh shortcut should exist; people here are installing NVM for the first time. Also, the install script should probably tell people (if they already have nvm) that running the latest/install.sh could override their existing installation; regardless, this is an nvm problem, not ours. I don't want to pollute this repository with one piece of version manager-specific code and add yet another piece that needs to be maintained and is dependent on NVM infrastructure or something else.
I would also be against that; I don't want to add specific code to one version manager. This is not an us problem, and the solution shouldn't be at the responsibility of the Node.js website... Sorry Jordan :/ |
|
It's a problem for nvm users, which makes it a node problem. I can certainly make the automation myself, but it'll still be a PR that needs manual review. In other words, it feels like you're prioritizing ideological code purity over user's needs here. (as for "nvm infrastructure", it's openjs infrastructure, which is precisely as reliable as node's own infra since it's owned by the same organization) |
Although I'd be willing to accept a patch for that, given the version is stored within a said JSON file or something that we could read from during build time. |
I don't think it is fair to say it is a Node problem by being a constraint you're intentionally creating for your own users. No other version manager has such constraint on their install script (that we have listed here on the CodeBoxes)
I'm prioritising DX and removing nvm-specific knowledge as a burden to my contributors. Which also removes the burden on you...
Touche. |
|
Anyhow, I'm sorry, but I won't remove my blocker as this stands, at least now. Let me mutter internally with myself and reflect on this PR. Feel free to open an alternative PR in the meantime. |
|
To restate, the alternative approach is that I make a workflow on nvm's own repo, and have it submit an automated PR to this repo every time nvm does a release. If you'd prefer an automated PR, and a delay until a human gets around to landing and deploying the PR before users get the latest version of nvm, then I can certainly set that up in the meantime. It's somewhat pressing since this is stemming from user-reported issues. |
I think it is fine opening a Workflow in this repository that reacts on new releases to nvm/nvm. |
Description
This PR attempts to ensure that the version of nvm users are instructed to install is always the latest one.
I had to make the "get snippets" function async for this, but I eagerly create the promise that will hold the latest version, so that the user has to wait as little time as possible. Another alternative would be to convert this function into a react component, and immediately render a hardcoded nvm version, but then later rerender when the fetch promise returns. This would be a larger change, though, so I didn't start out going that route.
Validation
I'm not sure how to test this locally, but I'm happy to do so if someone can give me pointers :-)
Related Issues
N/A
Check List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.I'll write tests once the implementation direction is confirmed :-)