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

feat(npm): support updating packageManager field and Yarn binary #12088

Merged
merged 13 commits into from
Nov 1, 2021

Conversation

ylemkimon
Copy link
Contributor

Changes:

  • Set constraints based on the packageManager field, simulating the behavior of Corepack
  • Support updating the packageManager field in package.json
    • Update lockfile with the new version
    • Update the Yarn 2+ binary checked in to the repo

Context:

Node v16.9.0+ includes a new tool called Corepack (formerly pmm), to help with managing versions of package managers. It uses the packageManager field in package.json, which is in the form of {yarn,pnpm}@<exact version>.

I've tried to create a new manager, but the logic is too intertwined, e.g., the lockfile depends on the version of the package manager. I agree that the npm manager is too complex and I see a lot of opportunities to refactor, but it'd be out of scope for this PR.

Fixes #11368 if the packageManager is set.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository:

/cc @arcanis (core maintainer of Yarn and Corepack)

lib/manager/npm/extract/index.ts Outdated Show resolved Hide resolved
@rarkins rarkins requested a review from viceice October 20, 2021 11:36
rarkins
rarkins previously approved these changes Oct 20, 2021
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

looks good so far, but i like to move the contraints extraction after update phase right before lockfile update binary call is happen

lib/manager/npm/post-update/index.ts Outdated Show resolved Hide resolved
lib/manager/npm/post-update/index.ts Outdated Show resolved Hide resolved
lib/manager/npm/post-update/index.ts Show resolved Hide resolved
@ylemkimon
Copy link
Contributor Author

@viceice

i like to move move the contraints extraction after update phase right before lockfile update binary call is happen

Do you mean you'd like to move these

const yarnUpdate = upgrades.find(isYarnUpdate);
const yarnCompatibility = yarnUpdate
? yarnUpdate.newValue
: config.constraints?.yarn;
before generateLockFile() calls?

@rarkins rarkins marked this pull request as draft October 31, 2021 06:43
@viceice
Copy link
Member

viceice commented Oct 31, 2021

I'm not sure, in other managers we prefer to extract the contraints in updateArtifacts, but npm manafer is a bit special, so i'm not sure about the right place.

But the place right before the yarn / npm / pnpm call would be the right place

@ylemkimon ylemkimon marked this pull request as ready for review November 1, 2021 04:26
@ylemkimon
Copy link
Contributor Author

I think refactor can be done in another PR, to reduce complications.

@rarkins rarkins marked this pull request as draft November 1, 2021 07:33
@ylemkimon
Copy link
Contributor Author

@rarkins Could you let me know what changes are required?

@rarkins
Copy link
Collaborator

rarkins commented Nov 1, 2021

@ylemkimon I thought you planned to do some refactoring first and then resume this PR?

@ylemkimon
Copy link
Contributor Author

ylemkimon commented Nov 1, 2021

@rarkins Nope, I was planning to do them after this PR gets merged 😄

@rarkins
Copy link
Collaborator

rarkins commented Nov 1, 2021

@viceice wdyt?

@viceice
Copy link
Member

viceice commented Nov 1, 2021

@rarkins Nope, I was planning to do them after this PR gets merged 😄

LGTM, please don't forget 😉

@rarkins rarkins marked this pull request as ready for review November 1, 2021 11:20
@rarkins rarkins enabled auto-merge (squash) November 1, 2021 11:26
@rarkins rarkins merged commit 46d996c into renovatebot:main Nov 1, 2021
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 28.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

latipun7 added a commit to latipun7/library that referenced this pull request Nov 1, 2021
Updating `yarn` binaries finally supported as of renovatebot/renovate#12088
latipun7 added a commit to latipun7/library that referenced this pull request Nov 1, 2021
Updating `yarn` binaries finally supported as of renovatebot/renovate#12088
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support updating Yarn binary
4 participants