-
-
Notifications
You must be signed in to change notification settings - Fork 198
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 Yarn to v4 #3612
Bump Yarn to v4 #3612
Conversation
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@metamask/approval-controller@0.0.0-use.local, npm/@metamask/controller-utils@0.0.0-use.local |
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
- path: .yarn/plugins/@yarnpkg/plugin-constraints.cjs | ||
spec: "@yarnpkg/plugin-constraints" | ||
|
||
yarnPath: .yarn/releases/yarn-3.3.0.cjs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is no longer necessary to store the Yarn binary in the repo, as Yarn can rely on the version that Corepack installs: yarnpkg/berry#4254
@@ -11,9 +13,3 @@ nodeLinker: node-modules | |||
plugins: | |||
- path: .yarn/plugins/@yarnpkg/plugin-allow-scripts.cjs | |||
spec: "https://raw.githubusercontent.com/LavaMoat/LavaMoat/main/packages/yarn-plugin-allow-scripts/bundles/@yarnpkg/plugin-allow-scripts.js" | |||
- path: .yarn/plugins/@yarnpkg/plugin-workspace-tools.cjs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All plugins are enabled by default, so there is no need to store these in the repo: yarnpkg/berry#3591
This PR is blocked on bumping to Node 18, but we may also have to make a change to the CI config as the |
a978a06
to
82539c5
Compare
2126c85
to
ef75d97
Compare
3b66efd
to
f54eb82
Compare
@@ -1,6 +1,10 @@ | |||
compressionLevel: mixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These options were added automatically by Yarn 4. As far as I can see, they disable default behavior that would otherwise be turned on:
@@ -88,7 +88,7 @@ | |||
"typescript": "~4.9.5", | |||
"yargs": "^17.7.2" | |||
}, | |||
"packageManager": "yarn@3.3.0", | |||
"packageManager": "yarn@4.2.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the latest version of Yarn. It's slightly further than what the module template has, but I will submit a PR to upgrade it there.
b35e280
to
88ffa29
Compare
This allows us to use newer features of Yarn. For instance, we will be able to write constraints in JavaScript format rather than Prolog.
@metamaskbot publish-previews |
@metamaskbot publish-preview |
I need to double-check that the publishing workflows still work. I'll do that on a fork. |
I've confirmed that these changes don't break the publish workflows on a fork. You can see the run here: https://github.com/mcmire/core/actions/runs/9423928547/job/25963317224. Note that |
Ready for review. |
uses: actions/setup-node@v3 | ||
- uses: actions/checkout@v4 | ||
- name: Use Node.js | ||
uses: actions/setup-node@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange that we're calling setup-node
here twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitely strange, yes, but there is a reason for it. We need to call it once to be able to use Corepack to install Yarn (recent versions of Node ship with Corepack). However, we can't restore the Yarn cache in this step because we need the correct version of Yarn to do that, and we don't have it yet. So after we install Node once without restoring the Yarn cache, we then install Corepack. Then we run the setup-node
action again, this time restoring the Yarn cache.
The alternative for this would be to ship the Yarn binary with the repo as we did before instead of using Corepack. Lego seemed to not like this way because then we need to guarantee that the Yarn binary is correct everywhere (which we never did): MetaMask/metamask-module-template#243 (comment). But we could open up that discussion again.
See relevant module template PRs:
@@ -50,7 +67,7 @@ jobs: | |||
fail-on-cache-miss: true | |||
- name: Dry Run Publish | |||
# omit npm-token token to perform dry run publish | |||
uses: MetaMask/action-npm-publish@v4 | |||
uses: MetaMask/action-npm-publish@v5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Was there a reason for updating to v5 in this PR? Seems like a good idea but it wasn't obvious how that related to the Yarn v4 update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good question, I didn't highlight this. Yarn v4 now requires passing an --all
option to yarn workspaces foreach
if you want to iterate over all packages in the workspace. action-npm-publish
v4 didn't pass this option, but v5 does now: MetaMask/action-npm-publish@f861a2b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Resolved conflicts. |
@SocketSecurity ignore npm/@metamask/controller-utils@10.0.0 Seems like a false negative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Explanation
This allows us to use newer features of Yarn, such as writing constraints in JavaScript format instead of Prolog.
References
Fixes #4383.
Changelog
(N/A)
Checklist