Breaking change in version 1.2.0 isn't represented with a major version increment

[paulgraff]

With the most recent release of this package, I'm no longer able to run eslint for my project without it failing. This is tough, because my team runs it on build as part of our CI tooling (currently using node version 5.12.0). Doesn't seem like NPM tries to resolve packages based on engine versions, either.

It looks like this was intentional– but I'm curious why there wasn't a major version increment for it?

As an aside: this particular package is one that we should be updating to used fix versions but haven't yet.

Thanks for maintaining this project! Would be glad to help out with any potential solutions you all would be interested in.

[michaelficarra]

Doesn't seem like NPM tries to resolve packages based on engine versions, either.

That would be really nice since it could "just work" by selecting the most recent version that meets the engine requirements. I'm sure someone has already had this idea before, but it might be worth looking into whether someone has suggested it to npm?

It looks like this was intentional– but I'm curious why there wasn't a major version increment for it?

That's correct. I decided not to major bump because the only versions of node we were dropping had fallen out of support already. For anyone still using these versions, they can pin esquery with a package-lock.json file. This way, many more users will see the benefit of the upgrade, since there's no actual API breakage. By the way, I highly recommend upgrading to a supported version of node, even on your CI systems.

Thanks for maintaining this project! Would be glad to help out with any potential solutions you all would be interested in.

Thank you so much, that means a lot.

[brettz9]

To my understanding, the package-lock.json only pins things for development use:

Per npm help package-locks:

It is highly recommended you commit the generated package lock to source control: this will allow anyone else on your team, your deployments, your CI/continuous integration, and anyone else who runs npm install in your package source to get the exact same dependency tree that you were developing on.

I see no mention of it impacting regular installs (whether direct or transitive).

I think to pin things for your regular users (direct or transitive), you would need to pin things by precise patch version and avoid ranges for a given package, the disadvantage there being that you'd have to push an update for each dependency update you wanted to pass on to your users.

While I see the appeal for a notion of full predictability of semver, even besides @michaelficarra 's well-reasoned, albeit non-standard, justifications for adding a practical conceptual layer over semver which allows for consideration of current Node support, there are other uncertainties in how semver is to be understood anyways (e.g., should a linting program that makes a mere bug fix call for a major bump if it may result in more errors for former users accustomed to the former lenience, should a polyfill adding a missing API consider that a bug fix or an enhancement, etc.). There really could, I think, be a policy page for every project on how it interprets semver.

If one really needs bulletproof certainty, I think one would simply need to avoid ranges (and hopefully use tools like npm-check-updates so one could provide frequent updates which exposed bug fixes for transitive dependencies).

[michaelficarra]

If one really needs bulletproof certainty, I think one would simply need to avoid ranges

This is insufficient and why package-locks are necessary. If a dependency of yours uses a range and its understanding of semver did not agree with one of its dependencies' understanding of semver, you will still observe breakage, even if you never use ranges yourself. Package-locks transitively pin dependencies.

[brettz9]

Good point, but as mentioned, package-locks apparently only have an effect for local installs.

I guess I should instead say that if you want bullet-proof certainty, bundle your dependencies. :-) (Ugh)

[mbroadst]

This has broken our build in the mongodb module, and while pinning to a version in a lock file is certainly one approach I think the fact that you broke support for a version of node (LTS or not) in a minor version is a strict violation of semver expectations.

We have gone through great pains to maintain support for Node v4 in this version of the driver, since we still have active users and don't want to break their applications. I wish you would reconsider rolling back the breaking change, and publishing a major release for this.

[mbroadst]

@michaelficarra actually, I wonder if you can help us now. It's not immediately obvious to me how to pin this in the lockfile, maybe you can give us some advice? This isn't a direct dependency of our project, but of eslint, and they have no intention of fixing this. Simply changing all references to esquery in the lock file doesn't work, so I was wondering what you had in mind? Thanks!

[michaelficarra]

@mbroadst For me, the decision comes down to a single trade-off: not breaking users of a version of node that is so old even the node maintainers do not support it vs every other user seeing the benefit of the improvements in this release and all future releases of this major. I personally prefer not to have the vast majority of users suffer for the benefit of the few laggards. If you are looking for maximum stability, it is never safe to use version ranges in your dependencies. Hopefully this experience has helped your project to become even more stable!

[michaelficarra]

@mbroadst It looks like you might want to look into npm-shrinkwrap.json? I don't have much experience with creating fully reproducible dependency installs using npm.

[ljharb]

npm-shrinkwrap and package-lock are identical, except the former is published (as such, published things should not use it).

@michaelficarra is the only "breaking" change the engines field? ie, does the code work the same on the same node versions?

[michaelficarra]

@ljharb Yes, there's no API breakage.

[mbroadst]

@michaelficarra I don't find that argument compelling.

We agree as a community to use semantic versioning to avoid these very problems: users who want to benefit from improvements to the code will update to the next major version of your project, users who cannot update at this time should not be forced to upgrade their entire application because you decided that they were "laggards". As maintainers we cannot make assumptions about why a user is using a particular version of a package, we can only help them make decisions by sticking to the versioning conventions we agree on as a community, issuing deprecations, and helping to better inform them about why they should upgrade.

As I previously mentioned, esquery is not a direct dependency of our project, so your comments about ranged dependencies don't apply here. Given the fact that eslint has no intention to make changes to software they released years ago (with an expectation that the dependencies they put in place then would follow semver), we the users are left with no path forward here. Yikes.

I just want to clarify, are you stating that esquery has no intention of following semver moving forward?

[michaelficarra]

@mbroadst Point me to the semver violation. semver talks about API, not platforms.

[mbroadst]

@ljharb @michaelficarra it is not just the engines field, we're getting the following error in Node Argon. I haven't dug into it at all, but it seems to indicate some unsupported ES6 feature introduced:

/home/mbroadst/Development/mongo/node-mongodb-native/node_modules/eslint/node_modules/esquery/dist/esquery.min.js:1
(function (exports, require, module, __filename, __dirname) { !function(e,t){"object"==typeof exports&&"undefined"!=typeof module?module.exports=t():"function"==typeof define&&define.amd?define(t):(e=e||self).esquery=t()}(this,(function(){"use strict";"undefined"!=typeof globalThis?globalThis:"undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof self&&self;function e(e,t){return e(t={exports:{}},t.exports),t.exports}var t=e((function(e,t){!function e(t){var r,n,s,o,a,i;function l(e){var t,r,n={};for(t in e)e.hasOwnProperty(t)&&(r=e[t],n[t]="object"==typeof r&&null!==r?l(r):r);return n}function u(e,t){this.parent=e,this.key=t}function c(e,t,r,n){this.node=e,this.path=t,this.wrap=r,this.ref=n}function p(){}function f(e){return null!=e&&("object"==typeof e&&"string"==typeof e.type)}function h(e,t){return(e===r.ObjectExpression||e===r.ObjectPattern)&&"properties"=

SyntaxError: Unexpected token [
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:373:25)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/home/mbroadst/Development/mongo/node-mongodb-native/node_modules/eslint/lib/util/node-event-generator.js:12:17)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)

[michaelficarra]

@mbroadst Sorry, let me be clear. The engines field in package.json reflects the supported platforms. For all supported platforms, there is no API breakage, e.g. semver minor.

[ljharb]

Supported platforms are determined by the .0.0 of the major line; so whatever 1.0.0 supported is what all 1.x must support.

[mbroadst]

@michaelficarra you're right, the semver specification does ultimately build upon a basic building block of API stability, but nothing is ever that simple. Perhaps we can squint a little bit here and consider the version of JavaScript supported by a runtime part of the API, in which case changing what version of node is supported in a minor version violates semver.

Additionally the npm documentation notes "If you introduce a change that breaks a package dependency, we strongly recommend incrementing the version major number; see below for details."

[ljharb]

The tradeoff you mentioned upthread is valid, but if you truly want the fix to reach the most people, then the best way to do that is to implement it in syntax that works in the most platforms - ie, the oldest possible ones, not merely the ones currently supported by node core. In other words, you've not prioritized "spreading the bugfix most widely", you've prioritized "using modern syntax, and not configuring your build process's targets widely" :-/

Is there a reason rollup couldn't be configured to output more compatible code, without needing to alter your source?

[michaelficarra]

The comments are coming in faster than I can respond!

@ljharb

Supported platforms are determined by the .0.0 of the major line; so whatever 1.0.0 supported is what all 1.x must support.

I don't see that in the semantic versioning spec. It may well be a convention among some, but I am not aware of it.

@mbroadst

Perhaps we can squint a little bit here and consider the version of JavaScript supported by a runtime part of the API

We could, but again we have to consider that if we do this, it prevents people from getting updates.

@ljharb

In other words, you've not prioritized "spreading the bugfix most widely", you've prioritized "using modern syntax, and not configuring your build process's targets widely" :-/

Sure, that's fair. Until recently, this package supported node 0.10 (😮). @brettz9 sent a series of PRs (#81, #83, #84, possibly others) that modernised the project and at the same time required dropping support for these unmaintained node versions . I'm not sure why each one needed to drop support. I didn't worry about it much because the node versions are unmaintained. If you or @mbroadst would like to look into what we could do to bring back that support, I would accept PRs and publish a patch.

[brettz9]

I can prepare a PR to use rollup-plugin-babel so that the compiled files are backward-compatible. Will be a little more work to get travis conditionally installing older nyc, etc., to ensure things are indeed passing.

[mbroadst]

@brettz9 beat me to it, thanks for the PR 😄

[ljharb]

@michaelficarra the spec is unfortunately silent on platform support, but in every language ecosystem i'm aware of, platform support is pretty universally considered part of the API (the "engines" field is npm's way for you to document a restriction, and restrictions are part of your API as well).

Hopefully the PR ends up landing and going out as a patch, and this will be trivially resolved.

[mbroadst]

@michaelficarra can we clarify that rolling back the change and publishing a major version for this module is out of the question? @brettz9 has done a great job investigating an alternative solution, but as you can see its not simple, and it involves bringing a lot of relatively heavy tooling into your project which may increase the maintenance burden over time.

[michaelficarra]

@paulgraff @mbroadst Published v1.2.1 with node 0.10+ compatibility.