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.

Sign up for GitHub

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

Jump to bottom

Drop support for Node.js 16 #221

Merged
merged 4 commits into from
Nov 17, 2023
Merged

Drop support for Node.js 16 #221

merged 4 commits into from
Nov 17, 2023

Conversation

Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Sep 30, 2023 •
edited
Loading

@Mrtenz Mrtenz requested a review from a team as a code owner September 30, 2023 09:53
@Mrtenz Mrtenz mentioned this pull request Sep 30, 2023
Merged
@Mrtenz Mrtenz marked this pull request as draft October 2, 2023 09:56
@MajorLift
Copy link

We might want to update the engines.node field in package.json to >=18.0.0 as well.

@MajorLift
Copy link

Line 93-94 of constraints.pro also needs to be updated:

% The package must specify a minimum Node version of 18.
gen_enforced_field(WorkspaceCwd, 'engines.node', '>=18.0.0').

@Mrtenz Mrtenz mentioned this pull request Oct 23, 2023
Merged
@Mrtenz Mrtenz force-pushed the mrtenz/drop-nodejs-16-support branch from 1258eb1 to a28ea98 Compare November 1, 2023 12:06
@Mrtenz Mrtenz marked this pull request as ready for review November 1, 2023 12:06
@mcmire
Copy link
Contributor

mcmire commented Nov 1, 2023

On a surface level now that mobile is on Node 18 I agree with this. I guess this assumes that we want to go with #216, however? It seemed that the conversation about node.engines wasn't quite resolved (I know Mark raised one about that vs. .nvmrc in Slack a while back).

mcmire
mcmire previously requested changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Marking this as blocked by discussion in #216 and also in Slack.

@Mrtenz Mrtenz marked this pull request as draft November 2, 2023 09:48
@mcmire mcmire dismissed their stale review November 15, 2023 16:13

We have discussed the concerns in #216 and that PR is good to go. Hence this PR is good to go as well.

mcmire
mcmire previously approved these changes Nov 15, 2023
View reviewed changes
@mcmire
Copy link
Contributor

mcmire commented Nov 16, 2023

I've fixed the merge conflicts on this PR. I also took the liberty to update to 18.18 (the current minor version as of this writing).

@mcmire mcmire marked this pull request as ready for review November 17, 2023 17:35
@mcmire mcmire mentioned this pull request Nov 17, 2023
Closed
MajorLift
MajorLift approved these changes Nov 17, 2023
View reviewed changes
Copy link

@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 mcmire merged commit 2bad8b0 into main Nov 17, 2023
14 checks passed
@mcmire mcmire deleted the mrtenz/drop-nodejs-16-support branch November 17, 2023 23:44
@MajorLift MajorLift mentioned this pull request Jul 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MajorLift MajorLift MajorLift approved these changes

@NicholasEllul NicholasEllul NicholasEllul approved these changes

@mcmire mcmire mcmire left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mrtenz @MajorLift @mcmire @NicholasEllul