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

Node 17 support #1541

Merged
merged 3 commits into from Mar 25, 2022
Merged

Node 17 support #1541

merged 3 commits into from Mar 25, 2022

Conversation

pcarrier
Copy link
Contributor

@pcarrier pcarrier commented Feb 24, 2022

Node 17 declared compatible in all package.json and added in CI.

@pcarrier pcarrier marked this pull request as draft February 24, 2022 23:54
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 24, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@pcarrier pcarrier marked this pull request as ready for review February 24, 2022 23:57
@pcmanus
Copy link
Contributor

pcmanus commented Mar 1, 2022

So the changes clearly looks good. And I don't have personal opinions about this.

But I believe our current and historical policy has been to only "support" LTS versions of nodes (even numbered) and let user pass --ignore-engines if they wanted to use something else. Tbc, I really don't mind changing that policy, now or later, but I'll ping @abernix and @trevor-scheer just to be sure they don't have things they want to add here.

I'll simply point out that this means we'll run more CI on every PR after this. I have no clue if we care (and of course, we could also allow 17 in the package.json but not run it in CI as an alternative).

abernix
abernix previously requested changes Mar 2, 2022
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

No objection, but please hook it up to the other requires in the CircleCI workflows section (can't comment on it in the PR because it's out of range). (Search for "JS: Node 16", look for oss/dry_run and oss/lerna_tarballs sections.

@abernix
Copy link
Member

abernix commented Mar 2, 2022

The things said above about historical policy are true. Odd versions of Node.js used to be much less stable than they are these days. That's changed to some degree, though. Our general pattern has been to only be testing 3 even-numbered (LTS) versions at a time. This puts us over that, but only for a couple months until May 1 when we probably will want to: add 18, drop 17 (because it immediately goes to unsupported) and also drop v12 (which will reach EOL)

@setchy setchy mentioned this pull request Mar 25, 2022
@pcarrier pcarrier requested a review from abernix March 25, 2022 19:11
@netlify
Copy link

netlify bot commented Mar 25, 2022

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 022ad74

@clenfest clenfest merged commit 41ccc43 into apollographql:main Mar 25, 2022
clenfest added a commit that referenced this pull request Mar 25, 2022
* Node 17 support

Co-authored-by: Chris Lenfest <clenfest@apollographql.com>
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.

None yet

5 participants