Skip to content
This repository has been archived by the owner on Nov 9, 2023. It is now read-only.

BREAKING: Update minimum Node.js version to 16 #20

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

legobeat
Copy link
Contributor

Required by update of @metamask/json-rpc-engine (#16)

@legobeat legobeat requested a review from a team as a code owner July 25, 2023 17:47
@legobeat legobeat requested a review from mcmire July 25, 2023 17:47
@socket-security
Copy link

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

Packages Version New capabilities Transitives Size Publisher
@types/node 17.0.45...17.0.23 None +0/-0 1.68 MB types

package.json Outdated
@@ -60,7 +60,7 @@
},
"packageManager": "[email protected]",
"engines": {
"node": ">=14.0.0"
"node": "16.20.1 || ^18.16.1 || >=20"
Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere we just use >=16. Is there a reason to be so specific with the supported version ranges?

Copy link
Contributor Author

@legobeat legobeat Jul 26, 2023

Choose a reason for hiding this comment

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

To not have to consider support 17 and 19 in the first place (17 went EoL before 16 FWIW, but not by much). A difference in outcome post-merge would be how to treat a hypothetical regression affecting only 17.x or 19.x. On the flip side, what's the upside of maintaining support for past odd-numbered releases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed now that the leading version was missing the ^, which was an oversight. Updated, and also dropped the least significant part (it's really the feature-release being relevant to consider for constraining IMO)

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, fair enough. I ask because it differs from our template. Perhaps we should migrate this change there as well?

RE: the lack of support for v16 below v16.20, I'm guessing that's just an attempt to disregard older versions as well, and associated bugs? Or is there another reason for that minimum, e.g. a feature we rely on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasoning: "We get the latest features for v16 and won't have to consider what's in which minor for future development" (It's happened every now and then that language features get introduced in LTS minors so this aims at limiting that aspect)

Basically devs will spend less time on https://node.green/.

@legobeat legobeat requested review from Gudahtt and a team and removed request for a team and Gudahtt July 26, 2023 05:28
Required by update of `@metamask/json-rpc-engine` (MetaMask#16)
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!

@legobeat legobeat merged commit 74cc342 into MetaMask:main Jul 26, 2023
16 checks passed
@legobeat legobeat deleted the node-16 branch July 27, 2023 00:26
legobeat added a commit that referenced this pull request Jul 27, 2023
@legobeat
Copy link
Contributor Author

Gah. Fat-fingered the merge @Gudahtt, sorry, this should wait for #21 .

#22 .

@legobeat legobeat mentioned this pull request Jul 27, 2023
legobeat added a commit that referenced this pull request Jul 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants