Added option to enable corepack #901

JP250552 · 2023-11-21T19:47:51Z

Description:

Updated version of corepack support due to the original author not having time to refactor the original PR

Adds corepack option in which workflows can specify true or a list of package managers to enable corepack accordingly.

Related issue:

Supersedes Option to enable corepack #651
Closes Corepack Support #531

Check list:

Mark if documentation changes are required.
Mark if tests were added or updated to cover the changes.

Co-authored-by: Steven <steven@ceriously.com> Co-authored-by: Sayak Mukhopadhyay <mukhopadhyaysayak@gmail.com> Co-authored-by: Jacob Parish <jacob.parish.1@gmail.com>

michaelmior · 2023-12-11T19:29:27Z

This works great for my use case. Looking over the code quickly, it also looks like this shouldn't introduce any breaking changes. Hoping this can get merged!

ranisalt · 2023-12-14T15:49:47Z

Is there any point in making it optional? One can just ignore corepack if they don't want to use it. Everything else will still work as always.

rt-joe · 2023-12-15T17:43:57Z

Is there any point in making it optional?

The biggest imo is it's still marked experimental. It is opt-in and should remain explicitly opt-in until it's part of stable node installation.

One can just ignore corepack if they don't want to use it. Everything else will still work as always.

It will "transparently install [the package manager] if needed, and finally run it without requiring explicit user interactions" which can lead to unintended change in behavior compared to if it were not enabled. Also "Running npm install -g yarn doesn't work" which I assume is a fairly common command for those that don't have/want the pre-installed yarn that comes with github hosted runners (for example #182).

Source: https://nodejs.org/docs/latest-v20.x/api/corepack.html

Also just generally safer to be opt-in to have stronger guarantee that it doesn't break existing functionality and workflows.

ranisalt · 2023-12-15T17:51:58Z

@rt-joe makes perfect sense. In this case, keeping it behind an input boolean should be enough

danielbayley · 2023-12-18T13:48:08Z

Merging this would nicely simplify a few of my workflows quite a bit! Thanks @JP250552

ranisalt · 2023-12-19T09:06:20Z

src/util.ts

+  const corepackArgs = ['enable'];
+  if (input.length > 0 && input !== 'false') {
+    if (input !== 'true') {
+      const packageManagers = input.split(' ');


Maybe also trim the input and split by \s+ to avoid issues due to leading, trailing and double-spacing, and also supporting multiline

rt-joe · 2024-02-28T18:15:04Z

Anything I can help out on to get this merged in?

JP250552 · 2024-02-28T18:19:22Z

We are just waiting for reviews.

@dsame Would you be able to take a look?

jkowalleck

the input corepack is used here and there. But is it listed in /action.yml?

PS: yes it is - #901 (comment)

jkowalleck · 2024-03-08T12:13:09Z

docs/advanced-usage.md

+- uses: actions/setup-node@v4
+  with:
+    node-version: '18.x'
+    corepack: true


the input corepack is not listed in /action.yml
did you forget to add it there?

Yes it is - https://github.com/actions/setup-node/pull/901/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R28

brianespinosa · 2024-03-26T20:40:42Z

We are just waiting for reviews.

Hi @JP250552!

it looks like we got a couple more reviews and approvals that were needed. Is there anything I could potentially help out with here? Happy to get some of my team's resources on things if there is anything to help out with getting this in and included in an upcoming release.

Thank you for all of your effort maintaining this community Action!

rt-joe · 2024-03-27T16:24:02Z

@brianespinosa know anyone with repo write access that can review (and approve) this?

styfle · 2024-03-27T18:25:19Z

Note that Corepack may stop working with future versions of Node.js if this PR gets merged:

deps: remove corepack nodejs/node#51981

And we might not need to change gh actions if this other PR gets merged:

Feel free to add 👍 or 👎 to the linked PRs above.

rt-joe · 2024-03-28T13:18:29Z

@styfle thanks for pointing those out, it's definitely worth noting.

My 2 cents: those do seem far away from reaching social consensus (there's still ongoing debate as of hours ago), let alone from being integrated into node. With the latest active LTS (v20) having maintenance until April 2026 (and v22 active LTS just around the corner), I think corepack with node will be here to stay for a while.

I don't know if your comment was to imply that this PR should be held off on being merged, but if so, I think it solves a popular usecase for many users that won't go away anytime soon and can be properly addressed if/when the decision to include/exclude corepack in node is finalized.

NoahAndrews · 2024-03-28T15:14:16Z

Even if it ends up excluded, it should be a very small change to install corepack from npm instead of running corepack enable when it did happen.

mcmire · 2024-04-26T15:47:53Z

We use Yarn heavily for our projects and I think it would be useful to have this option. I also don't see Corepack being removed at any point, but if we are concerned about that, perhaps it's worth highlighting the experimental nature of Corepack in the README? I can post a suggestion if that makes sense.

mcmire · 2024-04-26T15:49:25Z

src/util.ts

+      const packageManagers = input.split(' ');
+      corepackArgs.push(...packageManagers);
+    }
+    await getCommandOutput(`corepack ${corepackArgs.join(' ')}`);


Looking back at previous PRs, it was suggested that if Yarn ends up being enabled, any existing Yarn installation (which is evidently present on GitHub runners) should be uninstalled first: #482 (review). Is this is a concern?

thought corepack would interop with existing yarn installations (could be wrong)

Yes, the existing Yarn installation can be left as is and it won't cause conflict with the installed Yarn from Corepack.

JP250552 and others added 4 commits November 21, 2023 13:02

Added option to enable corepack (#1)

2936fe8

Co-authored-by: Steven <steven@ceriously.com> Co-authored-by: Sayak Mukhopadhyay <mukhopadhyaysayak@gmail.com> Co-authored-by: Jacob Parish <jacob.parish.1@gmail.com>

test: updated tests

7c0fdd0

docs: node version

f9532b2

docs: actions versions

28c91a3

JP250552 requested a review from a team as a code owner November 21, 2023 19:47

docs: remove reference to experimental

d8a8b93

JP250552 mentioned this pull request Nov 21, 2023

Option to enable corepack #651

Open

2 tasks

threeal approved these changes Nov 22, 2023

View reviewed changes

smallsaucepan mentioned this pull request Dec 7, 2023

Upgrade to yarn v4 Turfjs/turf#2551

Closed

4 tasks

Merge remote-tracking branch 'upstream/main' into feature/corepack

802542b

ranisalt reviewed Dec 19, 2023

View reviewed changes

jrparish added 3 commits January 2, 2024 08:34

Merge remote-tracking branch 'upstream/main' into feature/corepack

c871b9d

fix: pr review adjustments

c73bf90

test: added e2e test, improved unit test

0c618ce

jordanshatford mentioned this pull request Feb 9, 2024

feat(ci): use corepack when setting up node in CI jordanshatford/youtube-downloader#184

Closed

rt-joe approved these changes Mar 7, 2024

View reviewed changes

jkowalleck suggested changes Mar 8, 2024

View reviewed changes

jkowalleck approved these changes Mar 8, 2024

View reviewed changes

rt-joe mentioned this pull request Apr 17, 2024

Corepack Support #531

Open

This was referenced Apr 25, 2024

Unvendor Yarn; favor Corepack for installation MetaMask/metamask-module-template#243

Open

ci: run corepack enable before installing dependencies MetaMask/metamask-module-template#244

Merged

mcmire reviewed Apr 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option to enable corepack #901

Added option to enable corepack #901

JP250552 commented Nov 21, 2023

michaelmior commented Dec 11, 2023

ranisalt commented Dec 14, 2023

rt-joe commented Dec 15, 2023 •

edited

ranisalt commented Dec 15, 2023

danielbayley commented Dec 18, 2023

ranisalt Dec 19, 2023 •

edited

rt-joe commented Feb 28, 2024

JP250552 commented Feb 28, 2024

jkowalleck left a comment •

edited

jkowalleck Mar 8, 2024

JP250552 Mar 8, 2024

brianespinosa commented Mar 26, 2024

rt-joe commented Mar 27, 2024

styfle commented Mar 27, 2024

rt-joe commented Mar 28, 2024 •

edited

NoahAndrews commented Mar 28, 2024 •

edited

mcmire commented Apr 26, 2024

mcmire Apr 26, 2024 •

edited

shinebayar-g Apr 26, 2024 •

edited

threeal Apr 29, 2024

Added option to enable corepack #901

Are you sure you want to change the base?

Added option to enable corepack #901

Conversation

JP250552 commented Nov 21, 2023

michaelmior commented Dec 11, 2023

ranisalt commented Dec 14, 2023

rt-joe commented Dec 15, 2023 • edited

ranisalt commented Dec 15, 2023

danielbayley commented Dec 18, 2023

ranisalt Dec 19, 2023 • edited

Choose a reason for hiding this comment

rt-joe commented Feb 28, 2024

JP250552 commented Feb 28, 2024

jkowalleck left a comment • edited

Choose a reason for hiding this comment

jkowalleck Mar 8, 2024

Choose a reason for hiding this comment

JP250552 Mar 8, 2024

Choose a reason for hiding this comment

brianespinosa commented Mar 26, 2024

rt-joe commented Mar 27, 2024

styfle commented Mar 27, 2024

rt-joe commented Mar 28, 2024 • edited

NoahAndrews commented Mar 28, 2024 • edited

mcmire commented Apr 26, 2024

mcmire Apr 26, 2024 • edited

Choose a reason for hiding this comment

shinebayar-g Apr 26, 2024 • edited

Choose a reason for hiding this comment

threeal Apr 29, 2024

Choose a reason for hiding this comment

rt-joe commented Dec 15, 2023 •

edited

ranisalt Dec 19, 2023 •

edited

jkowalleck left a comment •

edited

rt-joe commented Mar 28, 2024 •

edited

NoahAndrews commented Mar 28, 2024 •

edited

mcmire Apr 26, 2024 •

edited

shinebayar-g Apr 26, 2024 •

edited