Skip to content

Commit

Permalink
Require node 18.18 at least
Browse files Browse the repository at this point in the history
  • Loading branch information
@bluesmoon
bluesmoon committed Oct 30, 2023
1 parent 3d9e902 commit 8c673bd
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"files" : ["lib/", "data/", "test/","scripts/"],
"main" : "lib/geoip.js",
"repository" : { "type": "git", "url": "git://github.com/geoip-lite/node-geoip.git" },
"engines" : { "node": ">=5.10.0" },
"engines" : { "node": ">=18.18.0" },
"scripts": {
"pretest": "eslint .",
"test": "nodeunit --reporter=minimal test/tests.js",
Expand Down

8 comments on commit 8c673bd

@teliov
Copy link

@teliov teliov commented on 8c673bd Nov 2, 2023

Choose a reason for hiding this comment

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

Dropping a comment that this broke a number of builds. I think bumping node versions is always a potential breaking change and the version bump (from 1.4.7 to 1.4.8) does not indicate this

@bluesmoon
Copy link
Collaborator Author

@bluesmoon bluesmoon commented on 8c673bd Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@teliov It wasn't this commit that broke the build. I added a unit test a few commits ago that requires the latest data files, but since we aren't allowed to ship with those files, the test will be broken until I can add test file download to the tests.

@teliov
Copy link

@teliov teliov commented on 8c673bd Nov 5, 2023

Choose a reason for hiding this comment

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

Understood, though I was referring to builds of libraries/projects that require node-geoip but are running in environments where node < 18.18. The npm install wouldn't work out of the box anylonger.

@bluesmoon
Copy link
Collaborator Author

@bluesmoon bluesmoon commented on 8c673bd Nov 5, 2023 via email

Choose a reason for hiding this comment

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

@alfaproject
Copy link

Choose a reason for hiding this comment

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

Well, we can't update from node 16 yet, so this can't be installed atm:

error [email protected]: The engine "node" is incompatible with this module. Expected version ">=18.18.0". Got "16.20.2"

I think @teliov point is, forcing a major node version should be a major version update for this module, so version 2.0.0 for this module if node 18 REALLY is needed

@bluesmoon
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alfaproject I understand. I'm looking for a reasonable version that is > 0.6. As of d0672b6 I've set the minimum version required to 10.3 because as far as I can tell that is the lowest version that supports a reasonable subset of ecmascript 2018.

@alfaproject
Copy link

Choose a reason for hiding this comment

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

@bluesmoon there's no problem at all in increasing to node 18. I'd say that's reasonable. The issue is that you did it in a patch release 'x.y.patch' which usually signals just a bug fix. If you don't care about semver or semver-like version increases that's fine too but maybe worth a note in readme (;

@ajw015
Copy link

@ajw015 ajw015 commented on 8c673bd Mar 7, 2024

Choose a reason for hiding this comment

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

Yes, this should have been a Major version.

It also appears that releases aren't being tagged any more here, so it can be harder to track down what has changed in an update

Please sign in to comment.