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

Bump minimum yarn version suggested in package.json engines #9144

Closed
wants to merge 3 commits into from

Conversation

NicholasEllul
Copy link
Contributor

Description

A bug was fixed in yarn that affects versions below 1.22.22 (yarnpkg/yarn#9023). This bug can result in issues for developers attempting to yarn deduplicate with old versions of yarn. Adjusting this engines file will help hint at the correct versions to use.

Note that this will not enforce developers to use the correct version, it will only help inform it.

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

github-actions bot commented Apr 4, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.98%. Comparing base (285fadd) to head (4721da3).

Additional details and impacted files
@@              Coverage Diff              @@
##           yarn-1.22.22    #9144   +/-   ##
=============================================
  Coverage         45.98%   45.98%           
=============================================
  Files              1273     1273           
  Lines             31342    31342           
  Branches           3213     3213           
=============================================
  Hits              14414    14414           
  Misses            16079    16079           
  Partials            849      849           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leotm leotm mentioned this pull request Apr 4, 2024
16 tasks
@NicholasEllul NicholasEllul changed the base branch from main to yarn-1.22.22 April 4, 2024 19:40
@NicholasEllul NicholasEllul requested review from leotm and removed request for a team April 4, 2024 19:40
Copy link

sonarcloud bot commented Apr 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@@ -63,6 +63,8 @@ With the correct Node version installed, Yarn v1 can be installed sudo-less in y
npm install -g yarn
```

Ensure that the installed version is within the range of supported versions specified in the `engines` section of [package.json](https://github.com/MetaMask/metamask-mobile/blob/main/package.json).
Copy link
Contributor

@legobeat legobeat Apr 16, 2024

Choose a reason for hiding this comment

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

If we set the packageManager field in package.json, I think corepack enable should do the right thing for you. I have not tried it with yarn1 in this repo and made sure no other steps are required, though.

Copy link
Contributor

@leotm leotm Apr 17, 2024

Choose a reason for hiding this comment

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

added the packageManager field here #9143 (comment)

and discovered #9143 (comment)

figured nvm install (v18.18.2) adds old [email protected] under-the-hood (npm list -g) which sadly only checks packageManager is a valid semver, but doesn't enforce it

i tried bumping corepack to latest (npm install -g corepack) which is [email protected] and now the yarn version is enforced

➜  metamask-mobile git:(yarn-1.22.22) ✗ yarn
! Corepack is about to download https://registry.yarnpkg.com/yarn/-/yarn-1.22.2222222.tgz
? Do you want to continue? [Y/n]

and without packageManager, latest corepack downloads and auto-adds "packageManager": "[email protected]+sha1.someLongHash" before falling to the package.json engine check - not good! so we deffo want the packageManager field

and didn't need to run corepack enable after upgrading

so we might want to mention too to run npm install -g corepack after nvm install (or nvm use if already there)

then something like

you may still need to run corepack enable

or could include corepack enable in our yarn setup script instead

@@ -511,7 +511,7 @@
},
"engines": {
"node": "~18.18.2",
"yarn": "^1.22.22"
"yarn": ">=1.22.22 <2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is semantically equivalent to the previous version - is it actually more meaningful?

@NicholasEllul
Copy link
Contributor Author

Closing as this is not succeeded by Leo's PR

@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants