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

chore(download): remove references to corepack #6396

Closed

Conversation

ruyadorno
Copy link
Member

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

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.

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 the feature to be
stable before highlighting it on the downloads page.
@ruyadorno ruyadorno requested a review from a team as a code owner March 2, 2024 17:08
Copy link

vercel bot commented Mar 2, 2024

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

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@AugustinMauroy AugustinMauroy added the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Mar 2, 2024
Copy link
Contributor

github-actions bot commented Mar 2, 2024

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 🔗

Copy link
Contributor

github-actions bot commented Mar 2, 2024

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 💤 0 ❌ 0 🔥 4.417s ⏱️

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -1. People already don't know corepack. What purpose would this serve?

@bmuenzenmeyer
Copy link
Collaborator

I'm -1.

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

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node.js does include Corepack, I agree with Brian

@ovflowd
Copy link
Member

ovflowd commented Mar 3, 2024

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

Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both yarn and pnpm recommend their users to use corepack:

And their users have been uding corepack for managing yarn and pnpm versions.
Removing it from downloads will not be fair to those package managers and its users.

@darcyclarke
Copy link
Member

darcyclarke commented Mar 5, 2024

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

@ovflowd
Copy link
Member

ovflowd commented Mar 5, 2024

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 :)

@aduh95
Copy link
Contributor

aduh95 commented Mar 5, 2024

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.

@ovflowd
Copy link
Member

ovflowd commented Mar 5, 2024

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
Copy link
Member

ovflowd commented Mar 5, 2024

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

@aduh95
Copy link
Contributor

aduh95 commented Mar 5, 2024

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

@ovflowd
Copy link
Member

ovflowd commented Mar 5, 2024

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
Copy link
Member

ovflowd commented Mar 5, 2024

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 ovflowd closed this Mar 5, 2024
@darcyclarke
Copy link
Member

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 #).

@ovflowd
Copy link
Member

ovflowd commented Mar 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants