Skip to content

ci: stop testing against NodeJS v14, v16#849

Merged
kfcampbell merged 11 commits intomainfrom
remove-eol-node-versions
Jun 12, 2023
Merged

ci: stop testing against NodeJS v14, v16#849
kfcampbell merged 11 commits intomainfrom
remove-eol-node-versions

Conversation

@wolfy1339
Copy link
Copy Markdown
Member

@wolfy1339 wolfy1339 commented May 13, 2023 •
edited
Loading

BREAKING CHANGE: Drop support for NodeJS v14, v16

@wolfy1339 wolfy1339 added maintenance Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR Type: Breaking change Used to note any change that requires a major version bump and removed maintenance labels May 13, 2023
@wolfy1339 wolfy1339 force-pushed the remove-eol-node-versions branch from 7731289 to 1018e9b Compare May 13, 2023 14:55
@wolfy1339 wolfy1339 marked this pull request as draft May 13, 2023 14:56
@wolfy1339 wolfy1339 force-pushed the remove-eol-node-versions branch 6 times, most recently from d4f5f23 to 08fda2f Compare May 13, 2023 15:52
@wolfy1339 wolfy1339 marked this pull request as ready for review May 13, 2023 16:05
gr2m
gr2m reviewed May 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@gr2m gr2m 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.

I think I'd remove both node 14 and node 16. Node 16 is nearly out of maintenance: https://nodejs.dev/en/about/releases/. If anyone complaints, they can still use the older versions and we can still accept patches for older versions. That will save us another breaking change release within a few months

Also when doing breaking changes, I'd always look for deprecations and remove them as well.

/cc @octokit/js

@wolfy1339
Copy link
Copy Markdown
Member Author

wolfy1339 commented May 13, 2023

I think I'd remove both node 14 and node 16. Node 16 is nearly out of maintenance: https://nodejs.dev/en/about/releases/.

I hadn't really thought of Node 16

If anyone complaints, they can still use the older versions and we can still accept patches for older versions. That will save us another breaking change release within a few months

Good point. I'll update my octoherd script and run it again with these changes

@G-Rath
Copy link
Copy Markdown
Member

G-Rath commented May 13, 2023

Inversely: why are we dropping support for v14? I know that it'll mean we might not have to transpile as much, but I wouldn't expect the wins to be that great so unless there's a particular feature or dependency we know we can be using with v14/v16, or there's another breaking change we want to land (which might be #841?), is there a good reason to drop support for these versions?

@wolfy1339
Copy link
Copy Markdown
Member Author

wolfy1339 commented May 14, 2023 •
edited
Loading

We will still support those versions with bug fixes for the near future.

Dropping support means easier transition to ESM, and no need to keep polyfills for the fetch API.

It just doesn't make much sense to support versions that are EOL

There was a discussion around this briefly, https://github.com/orgs/octokit/discussions/21

@wolfy1339 wolfy1339 changed the title ci: stop testing against NodeJS v14 ci: stop testing against NodeJS v14, v16 May 14, 2023
@wolfy1339 wolfy1339 force-pushed the remove-eol-node-versions branch from 08fda2f to 541bcc8 Compare May 14, 2023 02:00
@G-Rath
Copy link
Copy Markdown
Member

G-Rath commented May 14, 2023

We will still support those versions with bug fixes for the near future.

In that case we should be testing against them? You either are or are not supporting the versions right? 🙂

There was a discussion around this briefly, https://github.com/orgs/octokit/discussions/21

It doesn't look like anything was ever actually agreed from that discussion - I've posted a comment with my thoughts on it.

@wolfy1339
Copy link
Copy Markdown
Member Author

wolfy1339 commented May 14, 2023

In that case we should be testing against them? You either are or are not supporting the versions right? 🙂

As gr2m said, we can support them with patches in maintenance releases. In the current release, we won't be supporting them

If anyone complaints, they can still use the older versions and we can still accept patches for older versions.

@wolfy1339 wolfy1339 mentioned this pull request May 15, 2023
Merged
@wolfy1339 wolfy1339 mentioned this pull request May 15, 2023
Merged
kfcampbell
kfcampbell previously approved these changes May 19, 2023
View reviewed changes
@nickfloyd
Copy link
Copy Markdown
Contributor

nickfloyd commented May 23, 2023

https://github.com/orgs/octokit/discussions/44

@kfcampbell kfcampbell mentioned this pull request Jun 9, 2023
Merged
@kfcampbell kfcampbell merged commit ea6f1f9 into main Jun 12, 2023
@kfcampbell kfcampbell deleted the remove-eol-node-versions branch June 12, 2023 21:17
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 12, 2023

🎉 This PR is included in version 12.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gr2m gr2m gr2m left review comments

+1 more reviewer

@kfcampbell kfcampbell kfcampbell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

released Type: Breaking change Used to note any change that requires a major version bump Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Projects

🧰 Octokit Active
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wolfy1339 @G-Rath @nickfloyd @gr2m @kfcampbell