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 min Node version to 18.18; use LTS for dev #3611

Merged
merged 9 commits into from
May 31, 2024

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Dec 1, 2023

Explanation

Now that both extension and mobile are using Node 18, we can follow suit.

Changelog

For all packages

  • BREAKING: Bump minimum Node version to 18.18

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

@mcmire mcmire requested review from a team as code owners December 1, 2023 22:54
@mcmire
Copy link
Contributor Author

mcmire commented Dec 1, 2023

Note that this would require major versions for everything all over again, so there's no immediate urgency to merge this, but I want to go ahead and start the conversation :)

@mcmire mcmire force-pushed the bump-all-packages-to-node-18 branch from cb780c4 to a978a06 Compare December 1, 2023 23:03
@legobeat
Copy link
Contributor

legobeat commented Dec 2, 2023

@mcmire
Copy link
Contributor Author

mcmire commented Dec 2, 2023

Thank you! I will work through that list then.

@mcmire mcmire marked this pull request as draft December 5, 2023 02:23
@mcmire
Copy link
Contributor Author

mcmire commented Dec 5, 2023

Setting this to draft in the meantime.

@mcmire
Copy link
Contributor Author

mcmire commented Dec 5, 2023

I've created tickets for the above repos, aside from metamask-desktop (it's been put on ice, so I don't think we have to worry about that right now).

@Gudahtt
Copy link
Member

Gudahtt commented Jan 10, 2024

Shouldn't we bump the minimum version here first, before bumping the dependencies? We'd want to increase the minimum from the bottom-up, starting at the clients, then here, then to dependencies. Otherwise v16 support will effectively be dropped here prior to the supported range changing.

@Gudahtt
Copy link
Member

Gudahtt commented Jan 10, 2024

Oh I see, sorry I misunderstood. That list is for projects depending on this repository, similar to extension/mobile, so yes we'd want to update those first.

@mcmire
Copy link
Contributor Author

mcmire commented Jan 10, 2024

@Gudahtt (Oops I deleted my comment thinking I had misread @legobeat's comment) I think what @legobeat is saying is that the packages he linked to depend on some packages in the core monorepo, i.e., the core packages are the dependencies.

@mcmire
Copy link
Contributor Author

mcmire commented Jan 25, 2024

I've updated this PR so that it is ready to go when we merge MetaMask/smart-transactions-controller#249.

@mcmire mcmire marked this pull request as ready for review February 9, 2024 17:40
@mcmire
Copy link
Contributor Author

mcmire commented Feb 9, 2024

I've merged in changes from main and fixed lint violations. Ready for review again.

Gudahtt
Gudahtt previously approved these changes Feb 9, 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!

@MajorLift
Copy link
Contributor

In other packages we've been using "^18.18 || >=20" instead of ">=18.18 || >=20" in the engines field and yarn constraints. Are we purposefully going for a stronger constraint in core?

@mcmire
Copy link
Contributor Author

mcmire commented Feb 9, 2024

@MajorLift No, this was just an oversight 🤦🏻 I've fixed the versions to actually follow the module template.

MajorLift
MajorLift previously approved these changes Feb 9, 2024
Copy link
Contributor

@MajorLift MajorLift 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 Feb 9, 2024

@Gudahtt mentioned that we may want to merge #3908 before merging this PR. Adding DO-NOT-MERGE in the meantime.

@mcmire
Copy link
Contributor Author

mcmire commented Mar 15, 2024

This branch became stale, so I rebased it against the latest changes in main, and squashed all of the commits.

Merging this will kick off another round of breaking changes. Feel free to approve this, but we will see if we can pair this with some other breaking changes that we want to make anyway.

Copy link

socket-security bot commented Mar 15, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@mcmire mcmire requested a review from a team as a code owner April 18, 2024 23:24
legobeat
legobeat previously approved these changes Apr 25, 2024
@mcmire
Copy link
Contributor Author

mcmire commented Apr 30, 2024

Update on this PR: This is something that we still want to do, but we are waiting until we finish some fixes we want to make to the keyring controller, and I am also heads down on work to support multichain for the next few weeks or so. After we get through those, though, I can start the process to merge this PR and cut new major releases for everything.

@mcmire mcmire mentioned this pull request May 30, 2024
3 tasks
@mcmire mcmire requested a review from a team May 31, 2024 17:49
Copy link
Contributor

@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!

@mcmire mcmire merged commit 02ed6ee into main May 31, 2024
114 of 115 checks passed
@mcmire mcmire deleted the bump-all-packages-to-node-18 branch May 31, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants