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

chore: Migrate to Yarn 4, use JS constraints #156

Merged
merged 2 commits into from
Sep 27, 2024
Merged

chore: Migrate to Yarn 4, use JS constraints #156

merged 2 commits into from
Sep 27, 2024

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Sep 27, 2024

Migrates from Yarn 3 to 4 and replaces constraints.pro with JavaScript constraints via yarn.config.cjs. Migrating to Yarn 4 required a change to the Node / Yarn install flow in CI, which we solve by simply copypasting the workflows from the module template as of https://github.com/MetaMask/metamask-module-template/tree/570f6c2b80cb14fd61d5325131337401ae65c159.

@rekmarks rekmarks requested a review from a team as a code owner September 27, 2024 20:25
Copy link

socket-security bot commented Sep 27, 2024

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

Package New capabilities Transitives Size Publisher
npm/@yarnpkg/[email protected] None 0 8.8 kB yarnbot

View full report↗︎


const pullRequestTemplate = await getWorkspaceFile(
workspace,
'.github/pull_request_template.md',
Copy link
Member Author

Choose a reason for hiding this comment

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

The file name here is allcaps in the module template, and this blows up in Node 22.

Copy link

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -7,29 +7,47 @@ jobs:
prepare:
name: Prepare
runs-on: ubuntu-latest
strategy:
matrix:
node-version: [18.x, 20.x, 22.x]
Copy link
Contributor

Choose a reason for hiding this comment

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

Node.js v16 is still supported (as it very much should IMO, since there are existing downstreams still supporting it) so we ought to cover this version in tests, as long as it's still supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

Supported by whom? It's been EOL for almost a year: https://nodejs.org/en/about/previous-releases

Copy link
Contributor

Choose a reason for hiding this comment

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

By @metamask/eth-json-rpc-infura, which depends on this package. So this will prevent upgrade there until the same breakage is made downstream?


Supported by whom? It's been EOL for almost a year: https://nodejs.org/en/about/previous-releases

Just noting that in general:

If you only rely on public free support options, yes, it's EoL from perspective of Node.js maintainers. However, many users receive their Node.js versions from other maintainers and can and will still run older versions by virtue of external vendor maintenance! This typically includes bugfixes and security updates.

Some examples of popular usage of even older Node versions:

  • Debian bullseye is supporting Node.js v12 until 2026
  • Ubuntu 22.04 is supporting Node.js v12 until 2027 for Standard users; supposedly until 2032 for Pro Support customers)
  • This could also include Docker images based on the above dists, and vendors offering maintenance support for them
  • Maybe more practically relevant for real-world usage would be Enterprise Linux like RHEL and SUSE EL.

Just because Node.js devs say "this version is now dead" does not make it so. Even if we could choose to align with their support cycle, it's not a given.

@@ -7,29 +7,47 @@ jobs:
prepare:
name: Prepare
runs-on: ubuntu-latest
strategy:
matrix:
node-version: [18.x, 20.x, 22.x]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
node-version: [18.x, 20.x, 22.x]
node-version: [16.x, 18.x, 20.x, 22.x]

runs-on: ubuntu-latest
needs:
- prepare
strategy:
matrix:
node-version: [18.x, 20.x, 22.x]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
node-version: [18.x, 20.x, 22.x]
node-version: [16.x, 18.x, 20.x, 22.x]

runs-on: ubuntu-latest
needs:
- prepare
strategy:
matrix:
node-version: [18.x, 20.x, 22.x]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
node-version: [18.x, 20.x, 22.x]
node-version: [16.x, 18.x, 20.x, 22.x]

@rekmarks
Copy link
Member Author

@legobeat The minimum Node version was bumped in #154, so I will merge this and we can discuss whether to retain support for EOL versions in a separate issue or PR.

@rekmarks rekmarks merged commit 156c5c9 into main Sep 27, 2024
20 checks passed
@rekmarks rekmarks deleted the rekm/yarn-4 branch September 27, 2024 21:00
@legobeat
Copy link
Contributor

@legobeat The minimum Node version was bumped in #154, so I will merge this and we can discuss whether to retain support for EOL versions in a separate issue or PR.

Sire, I missed that that had just been created and merged prior. Just for visibility and future consideration: #156 (comment)

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.

3 participants