nodejs.org icon indicating copy to clipboard operation
nodejs.org copied to clipboard

chore(download): remove references to corepack

Open ruyadorno opened this issue 11 months ago • 5 comments

Description

There are multiple ongoing discussions at the moment around the future of corepack, with that in mind I believe it would be more prudent to wait for it to be stable before highlighting it on the downloads page.

Validation

Copy text only.

Related Issues

  • https://github.com/nodejs/node/pull/51886
  • https://github.com/nodejs/node/issues/51931
  • https://github.com/nodejs/node/issues/50963

Check List

  • [ ] I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • [ ] I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
  • [ ] I have run npx turbo format to ensure the code follows the style guide.
  • [ ] I have run npx turbo test to check if all tests are passing.
  • [ ] I've covered new added functionality with unit tests if necessary.

ruyadorno avatar Mar 02 '24 17:03 ruyadorno

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Mar 2, 2024 5:09pm

vercel[bot] avatar Mar 02 '24 17:03 vercel[bot]

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🔴 67 🟢 98 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 98 🟢 100 🟢 92 🔗
/en/about/previous-releases 🟢 99 🟢 96 🟢 100 🟢 92 🔗
/en/download 🟢 100 🟢 97 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 96 🟢 92 🟢 92 🔗

github-actions[bot] avatar Mar 02 '24 17:03 github-actions[bot]

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 85%
80.18% (433/540) 79.65% (137/172) 73.07% (76/104)

Unit Test Report

Tests Skipped Failures Errors Time
88 0 :zzz: 0 :x: 0 :fire: 4.417s :stopwatch:

github-actions[bot] avatar Mar 02 '24 17:03 github-actions[bot]

I'm -1.

Agree - the site content should reflect the current state of the project IMO.

bmuenzenmeyer avatar Mar 03 '24 05:03 bmuenzenmeyer

Although I understand that there are revolving discussions around the future of corepack.

I personally feel that it is important to highlight that Node comes with such binary bundled.

corepack, npm and node are binaries that come bundled within a Node installation

ovflowd avatar Mar 03 '24 14:03 ovflowd

tbqh I think you should remove the reference to npm as well. I say that because it doesn't seem like the references get updated when you select different versions of Node.js (& this inclusion statement should be accurate to the version you're considering downloading). I think adding the ability to update these inclusion references based on the Node.js version selected is a prerequisite to showing the information (or just remove both references since we don't make references to any other deps like V8 or OpenSSL & their corresponding versions anywhere). Instead of making these two deps special in some way, I think you document all or none (& - again - ensure they are accurate to the relative version).

darcyclarke avatar Mar 05 '24 04:03 darcyclarke

tbqh I think you should remove the reference to npm as well. I say that because it doesn't seem like the references get updated when you select different versions of Node.js (& this inclusion statement should be accurate to the version you're considering downloading). I think adding the ability to update these inclusion references based on the Node.js version selected is a prerequisite to showing the information (or just remove both references since we don't make references to any other deps like V8 or OpenSSL & their corresponding versions anywhere). Instead of making these two deps special in some way, I think you document all or none (& - again - ensure they are accurate to the relative version).

Sorry, but we do render the NPM version that is bundled with said Node.js version. Please test the UI carefully :)

ovflowd avatar Mar 05 '24 07:03 ovflowd

I must say, more than once I have searched the Node.js website trying to find which V8 version is bundled with which version, it'd useful to have this information. Not related to this PR though.

aduh95 avatar Mar 05 '24 07:03 aduh95

Also we do not render dependencies versions as their information is irrelevant for the average Node.js user. NPM and Corepack are not Node.js dependencies, imo; But tools that get bundled with Node.js. Corepack also does not have a version afaik, or at least not on index.json / nor I think it would be relevant for people to know the version.

But for NPM? That is definitely important information.

ovflowd avatar Mar 05 '24 07:03 ovflowd

I must say, more than once I have searched the Node.js website trying to find which V8 version is bundled with which version, it'd useful to have this information. Not related to this PR though.

Is it tho? The average Node.js user won't know what that means nor what is bundled with said V8 version. Even more as websites such as caniuse or MDN do not show V8 versions, just browser versions...

ovflowd avatar Mar 05 '24 07:03 ovflowd

The average Node.js user won't know what that means nor what is bundled with said V8 version

That seems like a hard to prove statement, we can only guess. From my own experience (back at a time where I wasn't involved in core), I was well-aware that JS syntax and built-in were provided by V8, and if I wanted to use say optional binding, or async/await, I knew I had to find which V8 version was running, and was a bit disappointed to find out I had to download each version of Node.js and run node -p process.versions.v8 to find out.

Even more as websites such as caniuse or MDN do not show V8 versions, just browser versions...

I don't think it's a very niche knowledge to know the version of V8 is the version of Chrome divided by 10.

Whatever, I'm just sharing my experience, I'm not trying to convince you, and it doesn't matter to me anymore because I have since learn how to use git to get that information. It seems pointless to hide that information away just because we think folks won't use it (the same argument could be made for NPM, most user won't know or care what a specific version of NPM means for them).

aduh95 avatar Mar 05 '24 07:03 aduh95

That seems like a hard to prove statement, we can only guess. From my own experience (back at a time where I wasn't involved in core), I was well-aware that JS syntax and built-in were provided by V8, and if I wanted to use say optional binding, or async/await, I knew I had to find which V8 version was running, and was a bit disappointed to find out I had to download each version of Node.js and run node -p process.versions.v8 to find out.

Definitely hard to prove on both ways, without running an experiment.

and if I wanted to use say optional binding, or async/await, I knew I had to find which V8 version was running, and was a bit disappointed to find out I had to download each version of Node.js and run node -p process.versions.v8 to find out.

I understand that part, the issue is, people will not easily know what V8 versions means which version of EcmaScript API. And if that's the intent, to let people know what version of EcmaScript we're supporting, shouldn't we then instead of rendering a V8 version, render ES API level? (I assume we could get that info based on the V8 version)

ovflowd avatar Mar 05 '24 10:03 ovflowd

Whatever, I'm just sharing my experience, I'm not trying to convince you, and it doesn't matter to me anymore because I have since learn how to use git to get that information. It seems pointless to hide that information away just because we think folks won't use it (the same argument could be made for NPM, most user won't know or care what a specific version of NPM means for them).

Well, we're not hiding, we simply don't have it at this point. But I'd say we're drifting away from the original point of this PR, which is: Should we remove Corepack references? And we got 3 Core Collaborators rejecting this change, meaning this PR is not going to land, so I'm closing it.

Regarding V8 or ES API level, @aduh95 do you mind opening an issue just for that? I'm open on having that being added on the website ;)

ovflowd avatar Mar 05 '24 10:03 ovflowd

Instead of closing the PR could we not be giving feedback on how to change it to be inline with everyone's suggestions? Corepack does have a version btw, & its origin is as a package distributed on the npm registry (so we should show the version #).

darcyclarke avatar Mar 05 '24 18:03 darcyclarke

Instead of closing the PR could we not be giving feedback on how to change it to be inline with everyone's suggestions? Corepack does have a version btw, & its origin is as a package distributed on the npm registry (so we should show the version #).

I think we're discussing that here first https://github.com/nodejs/nodejs.org/issues/6441

Since agreement was not reached upon here, I feel that doing an issue to keep track of alternatives + what we want to do to be more suitable.

We can always reopen this PR, it's just a state; I'm indifferent between keeping it closed or as a draft.

ovflowd avatar Mar 06 '24 11:03 ovflowd