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

Drop support for node.js engines below v10 #615

Merged
merged 3 commits into from
May 15, 2021

Conversation

xamgore
Copy link
Contributor

@xamgore xamgore commented Apr 4, 2021

It's a common practice to drop support for non-LTS Node versions, as it allows to reduce the maintenance burden. See more at:
https://nodejs.org/en/about/releases/

  • aws supports nodejs v14, v12, v10 (will not be available soon)
  • gcd supports nodejs v14, v12, v10, v8 (deprecated)

Also, I see artifacts in the code

something broken about domains in Node.JS v0.8 and mocha

or

function isFloatingPoint(n) {
    return n >= 0x8000000000000000 ||
        (Math.abs(n) < 0x4000000000000
         && Math.floor(n) !== n);

instead of Number.isSafeInteger(n) (appeared in v0.12).

My future PRs will be based on this.

It's a common practice to drop support for non-LTS Node versions,
as it allows to reduce the maintenance burden. See more at:
https://nodejs.org/en/about/releases/
@davesag
Copy link

davesag commented Apr 22, 2021

Note Node 16 came out today and the package.json specifies <= 15

I've tried it out with Node 16 and it works fine so expect just removing the upper cap of the engines section of package.json would be fine.

@xamgore xamgore mentioned this pull request Apr 26, 2021
@KristjanTammekivi
Copy link

@cressie176 Hi, sorry to mention directly but I haven't seen anything happening, would like to use node 16 but this is blocking

@gajus
Copy link

gajus commented May 3, 2021

@cressie176 If you add me as a collaborator to git / npm (@gajus), I am happy to help with this / future releases.

@gajus
Copy link

gajus commented May 11, 2021

@squaremo Could we get your help releasing this?

@squaremo
Copy link
Collaborator

I like being compatible with all versions of NodeJS ever (or close to), but I accept this now causes problems and everyone else moved on long ago. I have two questions before this proceeds:

  1. Are there usage stats for NodeJS versions? (alternatively: what proportion of the users of NodeJS does this change exclude?)
  2. What will people who don't use the right version of NodeJS see when they try to use this package?

@KristjanTammekivi
Copy link

$ npm i amqplib
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'amqplib@0.7.1',
npm WARN EBADENGINE   required: { node: '>=0.8 <=15' },
npm WARN EBADENGINE   current: { node: 'v16.1.0', npm: '7.11.2' }
npm WARN EBADENGINE }

This is what I currently see, it's only warnings and installs the package. I could swear that it used to refuse to install it.

As for statistics I don't think anyone really has them, however if you're using newest versions of amqplib I'd say you probably somewhat keep up with node.js releases. I don't really care much about the part of this PR but I would like the upper limit to be removed.

@kibertoad
Copy link
Collaborator

@squaremo This might provide some insights: https://twitter.com/kibertoad/status/1338886973256331267

@kibertoad
Copy link
Collaborator

Here's how up-to date numbers look (didn't get to do the proper reordering):

day 8 6 10 4 0.10 7 0.12 5 9 12 11 unknown 0.8 13 0.11 0.6 0.9 14 0.7 15 0.5 0.1 0.4 0.3 0.2 16
2021-05-12 4108 4963 5103 3673 2734 1871 1250 1619 2196 4437 1750 160 1227 1861 836 398 596 2897 268 1743 84 140 52 36 28 616

@kibertoad
Copy link
Collaborator

I think the tl;dr version is that everything below 4 can be dropped without a second thought. Dropping 4 and 6 would allow you to significantly modernize your code, though, and considering that likely the largest part of it is hopeless legacy whom noone updates anymore, not sure if it's worth to target that either.

@squaremo
Copy link
Collaborator

squaremo commented May 13, 2021

OK, let's bite that bullet. @xamgore can you alter the claim about supported versions in the README.md here too please, then I'll merge it.

@xamgore
Copy link
Contributor Author

xamgore commented May 14, 2021

@squaremo nailed it, thank you! 👍

Also, there is one more ready-to-merge PR, if you have free time, could you please check it out?

Copy link
Collaborator

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Thank you @xamgore

@squaremo squaremo merged commit 2dd1d57 into amqp-node:main May 15, 2021
@squaremo
Copy link
Collaborator

I went to make a release for this by tagging it, but forgot (again) that it needs a bit of preparation. Later today!

@squaremo
Copy link
Collaborator

Released in v0.8.0.

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.

6 participants