-
Notifications
You must be signed in to change notification settings - Fork 53
chore!: drop support for Node.js < v18 #230
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
base: main
Are you sure you want to change the base?
chore!: drop support for Node.js < v18 #230
Conversation
This is a breaking change, since it drops support for very old Node.js versions.
jsumners-nr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
timfish
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we want to merge the other PRs before this one so they end up on v2?
Then we might need a v2 branch so in the future we can still make bug fixes to v2?
|
@timfish yes, that sounds good to me. I think we will not need to support much more with v2 but let's see. |
|
@BridgeAR there are some merge conflicts to fix! |
|
@BridgeAR I pushed a merge from main that deals with conflicts. It might be worth a sanity check that I resolved the conflicts correctly. The conflicts were in 3 files: |
|
Does anyone have access to the full repo settings? I don't know if the |
|
The |
| - 18.5.0 | ||
| - 18.18.0 | ||
| - 18.19.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be dropping support for 18.x and 21.x as well, they've long been EOL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not possible for the main users of import-in-the-middle. They have other support ranges than Node.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe we could add a comment explaining where to find the list of versions to support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the package.json entry not solving that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I would be looking for something that explains why we need to support old versions, and how to maintain that list (e.g. a list of those "main users", or who to ask next time someone wants to update this list)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node 18 was EOL 2025-04-30. We have a different definition of "long" ago. ;)
Just my opinion: import-in-the-middle is used, at least, by a number of observability tools that are used in enterprises where updating software versions can take years. Leaving a bit of slack when dropping support for older Node.js versions can be helpful to downstream users. Or, from the pov of maintainers, can sometimes result in less work if supporting older Node.js versions for a while results in not ever having to do releases/maintenance on previous major versions.
Speaking for myself, downstream users of import-in-the-middle that I help maintain, and that still support Node.js 18 today are:
@opentelemetry/instrumentation- and, indirectly,
@elastic/opentelemetry-node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I don't have an opinion on which versions of Node.js this library should support; my point is that someone reading the code (e.g. my future self) might have the same reaction as me and think about removing support for EOL versions of Node.js. If we don't want that to happen, adding a comment could be an effective strategy. But it's just a nit, feel free to ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, if we drop v18 support Otel can't update from v2 and therefore won't get any more bug fixes unless we specifically patch and release for v2. Supporting v18 for now is less maintenance work.
|
I've updated the branch protection rules. Are there other checks that you want to require now? @BridgeAR should also be able to make the updates here: https://github.com/nodejs/import-in-the-middle/settings/rules/12251492 |
|
@trentm thank you for fixing the conflicts. It is LGTM. I will just not approve, since it feels wrong to approve my own PR, even if the latest changes are not done by me. |
| "version": "2.0.6", | ||
| "description": "Intercept imports in Node.js", | ||
| "engines": { | ||
| "node": ">=18" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That includes Node.js 19.x, 23.x, and 25.x that we are not testing on CI
This is a breaking change, since it drops support for very old Node.js versions.