feat(download): add fnm to package managers for all platforms #6667
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
MattIPv4
changed the title
MattIPv4
changed the title
|
@MattIPv4 do you mind adding it after NVM? So that NVM is still the default (first one) for macOS/Linux? All fine by me if it is the first one for Windows, since both on macOS/Linux Homebrew is also the 2nd one. |
|
@ovflowd sure, I can revert that, I kept it as a separate commit for that reason. That being said, what is the reasoning for wanting to keep NVM as the default? I outlined my justification for the change to fnm in the PR (it provides a faster (better) UX, and supports all platforms, so as the default folks get a consistent CLI no matter what platform they're on). |
Based on overall popularity and download stats... and my own discretion/personal bias (ie, maintainers of NVM are close-knitted to Node.js, which means we can better support/communicate with each other) But in the end, I'd say popularity is the primary reason. We don't have guidelines about the order of the package managers / or even which ones should be added, so meanwhile I'm just using my own discretion. We can come up with a document in the future, as suggested by @ljharb |
github-actions
bot
removed
the
github_actions:pull-request
|
Lighthouse Results
|
Unit Test Coverage Report
Unit Test Report
|
This reverts commit 8ed81a6.
I feel I should point out that this seems like a bit of a self-fulfilling prophecy if that is the main directive for what is listed first. If the most popular item is always listed first, it is never really going to give a chance to anything else to become the most popular, as its not what we're recommending by default (most users are probably just going to pick whatever we default to recommending). That being said, reverted the swap. |
@ljharb asked me to send this:
|
Yeh, I don't dispute that currently NVM is the most popular irrespective of what is default on the site for now. I just wish to note that the policy being that the most popular is set as the default seems like a good way to reinforce that the most popular remains the choice most folks will pick as it's the default. |
The switch statement was completely broken as far as I could tell -- it had no break statements, so if you were in an OS that matched the first condition, all the other conditions would get executed too. This made it impossible to use for fnm, as if the macOS/Linux condition matched, the Windows condition would get executed too, and so the wrong snippet was returned. |
Wouldn't doing the opposite also create the same effect? |
That was intentional. It was not broken. The lack of Please, revert the logic. Otherwise how do you think that currently nodejs.org is working fine? 😅 |
|
It seems to be working just fine on this preview? If I revert, how do you propose I add fnm support, as it cannot be present in the switch twice? I'd consider a switch without break statements, and needing the linter to be disabled, to be a giant anti-pattern. And if statement is far clearer in intention, and I'm pretty sure it still functions correctly... |
|
When a switch statement lacks breaks, it does NOT look for the next match, it runs every block after the one that matched, irrespective of the condition on the subsequent blocks. |
You are right, that seems a huge oversight from my side. I'm used with break/continue statement logic in other sort of loops, and forgot this doesn't apply to switch statements |
That is true, though I'd argue that giving the spotlight to tools that aren't the most popular is far less of a self-fulfilling prophecy than keeping it on the already established most popular option forever. |
|
Given the approvals, did you want to merge this as-is with the raster icon and the todo, and follow-up when a vector is available, or would you rather hold this as a draft for now? |
I understand your statement, but I want to keep biases off here. At this moment, the change of making fnm the first option should not be part of a PR adding fnm to the list. It's implicit/explicit bias, and I'm more than happy if you open a discussion thread to argument about that, although I believe these are highly personal arguments at best. |
We can wait for a vector image to be done :) |
|
@MattIPv4 are you waiting for someone specifically to make the SVG= |
|
I had pinged @Schniz in Schniz/fnm#798 (comment), but given the lack of response, I'll see if I can find some time myself to manually recreate it. |
Description
Draft pending a vector logo for fnm (Schniz/fnm#798 (comment))
This adds fnm to the package managers download page dropdown, with support for macOS + Linux + Windows.
Of note:
As part of this, I've added fnm as first in the list and changed the default to fnm. I feel this is appropriate as fnm is the only cross-platform option (and it in theory faster than NVM).
I updated the logic in the snippet generator, replacing the switch with a set of if statements. I am unsure why the switch had linting disabled for the lack of break statements, but this was causing switch blocks to be evaluated when their OS condition did not match, which broke adding FNM.
Validation
fnm appears in the package manager dropdown, with the correct snippet showing for Windows, and macOS/Linux.
All other OS/manager combinations generate the correct snippets as on the production site.
Related Issues
Resolves #6490
Check List
npx turbo formatto ensure the code follows the style guide.npx turbo testto check if all tests are passing.npx turbo buildto check if the website builds without errors.