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

Prefer using exclusively Corepack when possible #4254

Merged
merged 12 commits into from
Mar 29, 2022
Merged

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Mar 22, 2022

What's the problem this PR addresses?

Node 14 will soon be the new Node LTS. Since it ships with Corepack, we don't need to store the version file unless users already expect really want it.

How did you fix it?

The new code detects whether there's a yarnPath setting. If there is, it assumes that the version file will be needed. Otherwise, it just updates the packageManager field.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@arcanis arcanis added the major label Mar 22, 2022
@arcanis arcanis mentioned this pull request Mar 23, 2022
13 tasks
@merceyz
Copy link
Member

merceyz commented Mar 23, 2022

If there is no yarnPath value already present, how would you force Yarn to set it?

@arcanis
Copy link
Member Author

arcanis commented Mar 24, 2022

If there is no yarnPath value already present, how would you force Yarn to set it?

I've added a --yarn-path setting that will forcibly set yarnPath.

@arcanis arcanis requested a review from merceyz March 25, 2022 15:20
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

Needs tests

@ylemkimon
Copy link
Contributor

ylemkimon commented Mar 29, 2022

If Corepack is not enabled (which is by default), wouldn't this make Yarn classic (1.x) come into action? I think it should create a stub/placeholder binary to let people know Corepack is required for the time being, and ignore it in binary comparison.

@arcanis
Copy link
Member Author

arcanis commented Mar 29, 2022

If Corepack is not enabled (which is by default), wouldn't this make Yarn classic (1.x) come into action?

The PR title isn't accurate at the moment; Yarn will prefer using exclusively Corepack when possible, but if it isn't enabled (COREPACK_ROOT isn't in the environment) it'll fallback on yarnPath.

@arcanis arcanis changed the title Uses exclusively Corepack by default Prefer using exclusively Corepack when possible Mar 29, 2022
@arcanis arcanis requested a review from merceyz March 29, 2022 09:37
@ylemkimon
Copy link
Contributor

ylemkimon commented Mar 29, 2022

I was thinking of situations where a project created on an environment with Corepack enabled (hence no yarnPath) is shared with environments where Corepack is not enabled.

@arcanis
Copy link
Member Author

arcanis commented Mar 29, 2022

Yes, in that situation it won't work in the current state. To address that I'm considering adding Corepack support to the Yarn 1.x distribution (but only for the yarn binary, of course), so that any of corepack enable / npm install -g yarn would be as if Corepack was installed.

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

Successfully merging this pull request may close these issues.

3 participants