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

Add automatic corepack support for node v14 + #482

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Ethan-Arrowood
Copy link

@Ethan-Arrowood Ethan-Arrowood commented May 2, 2022

Description:

Executes corepack enable when the detected node version is 14+. This will enable support for the packageManager field for yarn and pnpm

Related issue:
#480

Check list:

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

src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
@Ethan-Arrowood
Copy link
Author

Struggling to get a test to pass. Insight appreciated

@Ethan-Arrowood Ethan-Arrowood marked this pull request as ready for review May 2, 2022 22:05
@Ethan-Arrowood Ethan-Arrowood requested a review from a team May 2, 2022 22:05
@Ethan-Arrowood
Copy link
Author

Probably wrong on the test, but since it is asserting the desired corepack output I'm marking this PR ready for review. Hopefully a maintainer more familiar with the test suite can provide some input on how to improve the tests.

@styfle styfle mentioned this pull request May 4, 2022
@shellscape
Copy link

Thanks for opening this PR. I support the idea.

Copy link

@styfle styfle left a comment

Choose a reason for hiding this comment

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

We also need to consider caching the result of corepack so that it doesn't need to install the package manger each time.

@Ethan-Arrowood
Copy link
Author

Added a check for the packageManager field. How do I fake a package.json file for the test?

I almost want to try writing an integration test for this, but would appreciate some input from the maintainers

@nickserv nickserv mentioned this pull request Jun 23, 2022
7 tasks
Copy link

@nickserv nickserv 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 in favor of using Corepack, but have some concerns about the implementation:

  1. This doesn't seem to support the @version syntax, which is automatically used in Yarn 3.1, and documented as experimental in Node. We'd have to at least allow for this syntax to set the package manager properly, though it would be nice to also parse the version and use it by default.
  2. GitHub runners already have Yarn installed. You're technically supposed to uninstall Yarn before enabling Corepack, as the global Yarn binary would conflict with Corepack's Yarn shim. We could try to fix this by either uninstalling Yarn before enabling Corepack, or removing Yarn from GitHub's runner (which would be a breaking change but would standardize behavior in the long run).

@beeequeue beeequeue mentioned this pull request Jul 17, 2022
2 tasks
@styfle
Copy link

styfle commented Aug 28, 2022

@nickmccurdy

  1. Can you explain more about this version syntax? I’m pretty sure “corepack enable will work with nearly all versions of Yarn.
  2. I think it would be hard to remove yarn from the runner so perhaps it should be uninstalled. What’s the best way to do that? npm uninstall -g yarn?

@nickserv
Copy link

  1. Can you explain more about this version syntax? I’m pretty sure “corepack enable will work with nearly all versions of Yarn.

Oh sorry, I see what you mean. It's not actually parsing the packageManager, but rather passing it to corepack.

  1. I think it would be hard to remove yarn from the runner so perhaps it should be uninstalled. What’s the best way to do that? npm uninstall -g yarn?

GitHub Actions seems to run npm install -g yarn ..., so yea, npm uninstall -g yarn should work.

@jtbandes
Copy link

@Ethan-Arrowood Are you planning to update this PR based on the feedback that has been provided? I know a lot of people are still interested in this feature. I see that CI is also failing currently.

@styfle
Copy link

styfle commented Apr 5, 2023

Looking at this PR again, it could be considered semver major because it will suddenly opt into corepack when the package.json is detected.

I'm starting to think that PR #651 is better solution because it could be considered semver minor due to the manual opt in.

That could prove valuable if anything changes in the future, since corepack is still technically experimental.

deining pushed a commit to deining/setup-node that referenced this pull request Nov 9, 2023
* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

---------

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants