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 Yarn to v4 #3612

Merged
merged 11 commits into from
Jun 27, 2024
Merged

Bump Yarn to v4 #3612

merged 11 commits into from
Jun 27, 2024

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Dec 1, 2023

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

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

Copy link

socket-security bot commented Dec 1, 2023

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] None 0 229 kB metamaskbot
npm/@metamask/[email protected] network 0 223 kB metamaskbot

🚮 Removed packages: npm/@metamask/[email protected], npm/@metamask/[email protected]

View full report↗︎

Copy link

socket-security bot commented Dec 1, 2023

👍 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: npm/@metamask/[email protected]

View full report↗︎

Next steps

Take a deeper look at the dependency

Take 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 package

If 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 risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

- path: .yarn/plugins/@yarnpkg/plugin-constraints.cjs
spec: "@yarnpkg/plugin-constraints"

yarnPath: .yarn/releases/yarn-3.3.0.cjs
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@mcmire
Copy link
Contributor Author

mcmire commented Dec 11, 2023

This PR is blocked on bumping to Node 18, but we may also have to make a change to the CI config as the setup-node step doesn't seem to be using Corepack to install Yarn 4.

@mcmire mcmire force-pushed the bump-all-packages-to-node-18 branch from a978a06 to 82539c5 Compare January 25, 2024 17:05
@mcmire mcmire force-pushed the bump-all-packages-to-node-18 branch from 2126c85 to ef75d97 Compare March 15, 2024 19:56
Base automatically changed from bump-all-packages-to-node-18 to main May 31, 2024 18:06
@mcmire mcmire force-pushed the bump-to-yarn-v4 branch 4 times, most recently from 3b66efd to f54eb82 Compare June 6, 2024 21:59
@@ -1,6 +1,10 @@
compressionLevel: mixed
Copy link
Contributor Author

@mcmire mcmire Jun 6, 2024

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",
Copy link
Contributor Author

@mcmire mcmire Jun 6, 2024

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.

@mcmire mcmire force-pushed the bump-to-yarn-v4 branch 3 times, most recently from b35e280 to 88ffa29 Compare June 6, 2024 22:10
This allows us to use newer features of Yarn. For instance, we will be
able to write constraints in JavaScript format rather than Prolog.
@mcmire
Copy link
Contributor Author

mcmire commented Jun 6, 2024

@metamaskbot publish-previews

@mcmire
Copy link
Contributor Author

mcmire commented Jun 6, 2024

@metamaskbot publish-preview

@mcmire mcmire marked this pull request as ready for review June 6, 2024 22:24
@mcmire mcmire requested a review from a team as a code owner June 6, 2024 22:24
@mcmire mcmire marked this pull request as draft June 6, 2024 22:24
@mcmire
Copy link
Contributor Author

mcmire commented Jun 6, 2024

I need to double-check that the publishing workflows still work. I'll do that on a fork.

@mcmire
Copy link
Contributor Author

mcmire commented Jun 7, 2024

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 publish-npm fails, but that's expected, as I don't have a token set up on the fork, so it's effectively doing dry-run twice, and the publish-npm action doesn't expect to be used that way.

@mcmire mcmire marked this pull request as ready for review June 7, 2024 22:49
@mcmire mcmire requested a review from a team June 7, 2024 22:51
@mcmire
Copy link
Contributor Author

mcmire commented Jun 7, 2024

Ready for review.

uses: actions/setup-node@v3
- uses: actions/checkout@v4
- name: Use Node.js
uses: actions/setup-node@v4
Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense!

Gudahtt
Gudahtt previously approved these changes Jun 26, 2024
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcmire
Copy link
Contributor Author

mcmire commented Jun 26, 2024

Resolved conflicts.

@mcmire
Copy link
Contributor Author

mcmire commented Jun 26, 2024

@SocketSecurity ignore npm/@metamask/[email protected]

Seems like a false negative.

@mcmire mcmire requested a review from a team June 27, 2024 17:57
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcmire mcmire merged commit 27dd62c into main Jun 27, 2024
116 checks passed
@mcmire mcmire deleted the bump-to-yarn-v4 branch June 27, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump Yarn to v4
2 participants