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

Release @2.82.0 broke semver minor version #2772

Closed
louisbuchbinder opened this issue Sep 19, 2017 · 50 comments
Closed

Release @2.82.0 broke semver minor version #2772

louisbuchbinder opened this issue Sep 19, 2017 · 50 comments

Comments

@louisbuchbinder
Copy link

Hi sorry to be a pain, but upgrading the Hawk dep broke the semver minor version. This is a breaking change as the new hoek uses const instead of var. Hawk uses Hoek, which uses const. This release should be major version @3.

const Hoek = require('hoek');
^^^^^
SyntaxError: Use of const in strict mode.
@rogers140
Copy link

Same here. Node version: 0.12.7 for historical reason.

@louisbuchbinder
Copy link
Author

tell me about it...

@rahul2983
Copy link

I am having a similar issue. Using node version 0.12.7 Please revert back to request version 2.81.0 or change the hawk dependency to version ~3.1.3

@boJackEden
Copy link

Same issue. Breaking my build. @rahul2983 quick update would save me a lot of hassle.

@rogers140
Copy link

We are using another module that depends on this, and in that module, it writes dependencies as request ^2.54.0. Things really beyond my control now... either we make our local copy of that module or just upgrade node (highly leverage here)

@dbine
Copy link

dbine commented Sep 20, 2017

+1 same issue. Please resolve per everyone's suggestions above.

@allardvanderouw
Copy link

I had the same problem, temporarily fixed it by changing my dependency from "request": "^2.81.0" to "request": "2.81.0"...

@impol
Copy link

impol commented Sep 20, 2017

npm-shrinkwrap.json

{
 "dependencies": {
  "request": {
   "version": "2.81.0"
  }
 }
}

@gogson
Copy link

gogson commented Sep 20, 2017

Lesson here is always fix your package versions, don't rely on people and semantic versioning myth.

@danielnitu
Copy link

Changed from "^2.78.00" to "2.78.00". Our server was just sending a 502 error. I agree with @gogson, fix your versions, don't rely on others.

@peterlundberg
Copy link

peterlundberg commented Sep 20, 2017

This broke our builds too as the new version of hawk uses let etc in its source which is as expected by node < 6 but also uglify used in many build pipes to minimize distributions. So while unintended this is a big surprise for many users of the library. At a minimum also engines in package.json should get updated.

If the security concerns (regarding boom) in #2748 are warranted perhaps there are other ways of getting this update? Hawk flagged this as breaking in mozilla/hawk#153

@kshay
Copy link

kshay commented Sep 20, 2017

Our build broke yesterday too due to upstream node-sass, jsdom, and probably others. We're working on workarounds now but it would be great if this could be reverted/fixed in this package.

@watson
Copy link
Contributor

watson commented Sep 20, 2017

Hopefully this can all teach us a lesson for the future: Any project that stops testing against a major version of Node.js should consider it a breaking change. Else you don't know if you by accident introduce breaking changes down the line. Then this issue would not have happened 😄

This project used to test both v0.10 and v0.12 in the .travis.yml file.

@mikeal
Copy link
Member

mikeal commented Sep 20, 2017

The solution to your problems is quite simple: STOP SUPPORTING v0.10 & v0.12.

Almost a year ago the Node.js project's maintenance window ended for pre v4 releases. This includes critical security updates and OpenSSL. It is irresponsible to continue to support and encourage people to run these versions of Node.js.

Bumping the major version would require every downstream user of request, which if you include deep dependencies is more than half the registry, to manually bump their major version in order to continue getting new releases of Node.js.

It's not fair to the people who are doing the right thing and keeping up to date with supported versions of Node.js to opt them out of updates without intervention so that people who are refusing to upgrade can continue to get fixes.

If you MUST support v0.10 and v0.12 then you can lock your package version while the rest of the ecosystem continues to get fixes, including security fixes like the ones in hawk.

It's quite simple, if you are running an old and insecure version of Node.js then you are no longer going to get security fixes from Node.js OR request, and its dependencies like hawk.

@mikeal mikeal closed this as completed Sep 20, 2017
@mikeal
Copy link
Member

mikeal commented Sep 20, 2017

BTW, these "breaks" all seem to be in tests on v0.10 and v0.12.

Remove travis support for those releases. They are unsupported by the Node.js project, they shouldn't be supported by your package.

@xepps
Copy link

xepps commented Sep 20, 2017

Whilst I agree that no one should be on an old version of node - that option is not always available.

Traditionally, if you want to drop support for something and introduce a breaking change, the way to do it is with a major version.

@mikeal
Copy link
Member

mikeal commented Sep 20, 2017

Whilst I agree that no one should be on an old version of node - that option is not always available.

Nobody is saying you have to upgrade to a new Node.js. Go ahead and run whatever you want forever.

But, you don't ALSO get dependency upgrades in perpetuity. If you want to run an old application on an old Node.js, lock your deps. Otherwise they are likely to break.

@mikeal
Copy link
Member

mikeal commented Sep 20, 2017

Traditionally, if you want to drop support for something and introduce a breaking change, the way to do it is with a major version.

I just don't consider a change "breaking" if it breaks on an unsupported version of Node.js.

@lxe
Copy link

lxe commented Sep 20, 2017

The solution to your problems is quite simple: STOP SUPPORTING v0.10 & v0.12.

It's the correct solution, no doubt about it, but it's far from being "quite simple" for reasons related to scale, organizational makeup, technical debt, prioritization of work, separation of organizational concerns, etc...

@mikeal
Copy link
Member

mikeal commented Sep 20, 2017

It's the correct solution, no doubt about it, but it's far from being "quite simple" for reasons related to scale, organizational makeup, technical debt, prioritization of work, separation of organizational concerns, etc...

I think we're conflating application upgrades and module upgrades.

Module can just drop old versions from their travis.yml file. That's pretty easy.

Applications are much harder, but if they decide not to upgrade they can simply lock down the versions of their deps and stop upgrading their deps.

@lxe
Copy link

lxe commented Sep 20, 2017

Applications are much harder, but if they decide not to upgrade they can simply lock down the versions of their deps and stop upgrading their deps.

That's true, but then the application won't receive speedy security updates and bug fixes. It's a bit of a balance...

I completely agree though that supporting legacy Node versions from a package maintainer's perspective is very difficult, and advising both the module and application maintainers to drop support for legacy Node versions is good advice.

@rogers140
Copy link

@mikeal I really think @xepps made the correct point. The issue becomes too hard to solve if old project is depending on some modules that are using some other modules that are using request. For example: googleapis -> gapitoken -> request ^2.54.0, in this case, if a project is using googleapis in node 0.12.7, they are screwed and there is no easy way to fix this, other than upgrading node (not always an option). So making this change in a major version like 3.0.0 or something like that would help a lot and resolve it.

@eliihen
Copy link

eliihen commented Sep 20, 2017

Please consider that version numbers are free, customers experiencing bugs in production is expensive. This caused me 1.5 hours of fumbling while prod was on fire. No one is going to care that your library is v15.0.0 if it means that dropping backward compat is done responsibly (with a semver major version).

I totally understand the position that 0.x is to be universally dropped, but that isn't done overnight. In my opinion the only responsible thing to do, especially when you have a large number of downstream projects, is to make dropping 0.x compat a major version regardless of 0.x's support state.

Even if one does not share this view, others will, and they deserve to be warned of the potential breaking changes in their library with the red flag of a major version.

@mikeal
Copy link
Member

mikeal commented Sep 20, 2017

Please consider that version numbers are free

Except when you have a large install base that only automatically upgrades minor releases. If we push a new major it would take years for maybe 10% of our users to be getting the number of updates they get now.

@hueniverse
Copy link
Contributor

hueniverse commented Sep 20, 2017

This is a case of who do you want to optimize the experience for: people who run on unsupported platforms or people who keep up with the bare minimum of staying on a supported node LTS version? I completely agree with @mikeal that the people we should care about more are those running supported versions.

The actual semver violation was 11 months ago here: 01aefdd

Dropping support should have been a major then when the contract changed. The same argument could have been made then though, but back then I would have been more supportive of those losing compatibility. A year later, now that request v2 has been re-contracted as node 4+, it's too late to make this argument for a major bump.

Also - doesn't npm complain when you install request on unsupported node version?!

<rant>
The module system is never a valid excuse for you to have poor understanding of your full stack dependencies. I am tired of people making the argument that they should be able to just use whatever random module from the npm registry they find and things should magically work. It's software made by humans. I review every single dependency of code I bring into my projects and all their dependencies. And I expect everyone to do the same. Sounds unrealistic? Maybe use a lot less imported crap that you probably don't need. Go lookup some of my old rants about tiny modules...

If your production environment was affect by this, you have a shit production setup. Period. You clearly don't know how to run node in a responsible manner. If you are still running node 0.x you are in completely uncharted territory - you are running unsupported garbage that is known to have bugs and security issues. At least this time the issue was clear and your shit environment stopped working. But you are as likely to hit really complex bugs in socket handling and memory leaks that a "semver compliant" version of request can expose that only exists in node 0.x.

If you are running an unsupported version of anything, you should stop upgrading everything! Own your mistakes for a change.
</rant>

@mikeal
Copy link
Member

mikeal commented Sep 20, 2017

Also - doesn't npm complain when you install request on unsupported node version?!

Yup :)

@dantman
Copy link

dantman commented Sep 25, 2017

To my knowledge, Debian has never back ported a single fix to an old versions of Node.js, much less all the dependencies an application might have.

I just gave an example of Debian backporting a fix to a security vulnerability that affects node. Debian splits up packages, i.e: the libraries nodejs uses (the biggest source of security issues) are separate packages instead of directly part of the nodejs package and they get security packages individually, which implicitly fixes the security issue in node. So the only time Debian needs to make a backport directly to the nodejs package that you'll see in the changelog for the nodejs package is if it's a security vulnerability directly in the nodejs source code created by one of the nodejs devs (a rare type). Would you care to give me an example of one of those, or another security vulnerability you'd like me to check the nodejs package for?

@mikeal
Copy link
Member

mikeal commented Sep 25, 2017

Great! Debian can do the work of backporting hawk to 0.10.0 and keeping a working fork of request for 0.10.x users, cause I'm not going to waste my time supporting EOL versions of Node.js.

If they want to support EOL software they can do all the work of supporting it, both in Node.js and in dependencies like request.

@jasonpincin
Copy link

So to summarize what's come out of this: Pin everything. Semver (within NPM at least) has an extremely finite window in which it has any meaning at all.

@dantman
Copy link

dantman commented Sep 25, 2017

So to summarize what's come out of this: Pin everything. Semver (within NPM at least) has an extremely finite window in which it has any meaning at all.

shrinkwrap/yarn is good to use in applications, it'll give you control over exactly what versions of libraries are installed and only upgrade them when you intend to upgrade packages.

However I wouldn't say that about semver or npm. The mass majority of packages used by people on npm stick to semver. For example the hawk package request uses where this error originally is thrown from bumped its major to 4.0.0 when it dropped support for nodejs@<4. It's just that request is one of the few packages that doesn't care to stick to semver when doing so. The only reason that hawk is throwing that error is because request started depending on it without a major release. So you can safely use ^ for most packages, but do have to be wary of a few black sheep like request.

@travi
Copy link

travi commented Sep 25, 2017

i think a better summary might be to not use dependencies that are marked as incompatible. even locking dependencies isn't ideal if you aren't first confirming compatibility.

for example, my builds call npm ls and check the status code to ensure things like peer-dependencies are compatible with each other. i do this even before i run my test suite to ensure i have a reasonable starting point.

i'm not sure if that particular command would fail for packages that are incompatible with the current node version (it does not fail for deprecations), but i would be surprised if there weren't a way to ensure that your project is not ignoring warning programmatically.

can anyone confirm if npm ls or a similar command could help identify node version compatibility so that others running into issues could take real action to improve their situation, even if upgrading isn't an option?

@jokeyrhyme
Copy link

Sometimes users just forget to update their Node.js, so I built these:

In my own CLI tools, I usually have these set to start bugging users if their version of Node.js is older than 90 days, and if their Node.js version doesn't match package.json:engines.node

@hueniverse
Copy link
Contributor

This will get even more fun next week when I publish new major versions of all my modules to only support node v8... :-)

@Laxmantln
Copy link

Below error/issue resolved with this solution:

Solution:

sed -i '27 a "request":"2.81.0",' /opt/apache-ambari-2.2.2-src/ambari-web/package.json

I just added specified "request" version(older version "2.81.0") in "package.json" file below problem resolved.

Here is my error:

[INFO]
[INFO] --- exec-maven-plugin:1.2.1:exec (compile-brunch) @ ambari-web ---
[DEBUG] Configuring mojo org.codehaus.mojo:exec-maven-plugin:1.2.1:exec from plugin realm ClassRealm[plugin>org.codehaus.mojo:exec-maven-plugin:1.2.1, parent: sun.misc.Launcher$AppClassLoader@5c647e05]
[DEBUG] Configuring mojo 'org.codehaus.mojo:exec-maven-plugin:1.2.1:exec' with basic configurator -->
[DEBUG] (f) basedir = /opt/apache-ambari-2.2.2-src/ambari-web
[DEBUG] (f) classpathScope = runtime
[DEBUG] (f) commandlineArgs = build
[DEBUG] (f) executable = brunch
[DEBUG] (f) longClasspath = false
[DEBUG] (f) project = MavenProject: org.apache.ambari:ambari-web:2.2.2.0.0 @ /opt/apache-ambari-2.2.2-src/ambari-web/pom.xml
[DEBUG] (f) session = org.apache.maven.execution.MavenSession@5ce4369b
[DEBUG] (f) skip = false
[DEBUG] (f) workingDirectory = /opt/apache-ambari-2.2.2-src/ambari-web
[DEBUG] -- end configuration --
[DEBUG] Executing command line: brunch build

/opt/apache-ambari-2.2.2-src/ambari-web/node_modules/ember-precompiler-brunch/node_modules/jsdom/node_modules/request/node_modules/hawk/node_modules/boom/lib/index.js:5
const Hoek = require('hoek');
^^^^^
Error: You probably need to execute npm install to install brunch plugins. SyntaxError: Use of const in strict mode.
at /opt/node-v0.10.44-linux-x64/lib/node_modules/brunch/lib/watch.js:433:19
at Array.map (native)
at loadDeps (/opt/node-v0.10.44-linux-x64/lib/node_modules/brunch/lib/watch.js:418:10)
at loadPackages (/opt/node-v0.10.44-linux-x64/lib/node_modules/brunch/lib/watch.js:438:15)
at /opt/node-v0.10.44-linux-x64/lib/node_modules/brunch/lib/watch.js:458:19
at /opt/node-v0.10.44-linux-x64/lib/node_modules/brunch/lib/helpers.js:529:16
at /opt/node-v0.10.44-linux-x64/lib/node_modules/brunch/lib/helpers.js:485:14
at /opt/node-v0.10.44-linux-x64/lib/node_modules/brunch/node_modules/read-components/index.js:263:16
at /opt/node-v0.10.44-linux-x64/lib/node_modules/brunch/node_modules/read-components/index.js:72:14
at Object.cb [as oncomplete] (fs.js:169:19)

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] Ambari Main ........................................ SUCCESS [ 3.970 s]
[INFO] Apache Ambari Project POM .......................... SUCCESS [ 0.591 s]
[INFO] Ambari Web ......................................... FAILURE [01:08 min]

@jsumners
Copy link

I'm late to the party, and I can't resist chiming in.

Everyone who is complaining about "semver not being adhered to", try reading the spec again. The spec deals specifically with the public API of the versioned item. How that API is implemented is not covered, as long as that API remains the same:

  1. Version 1.0.0 defines the public API. The way in which the version number is incremented after this release is dependent on this public API and how it changes.

http://semver.org/#spec-item-5

In fact, it's point number 1 in the summary of the spec.

Did request's API change? No? Then semver wasn't broken.

pcarranzav pushed a commit to balena-os/balena-supervisor that referenced this issue Nov 9, 2017
We've been using UglifyJS (the webpack default) so far, but this doesn't support ES6 and some dependency
updates are starting to cause builds to break (e.g. request/request#2772, which also happens to break
my builds in the multicontainer branch).

Here we switch to using babel-minify which is designed for ES2015 support. I had to disable the "mangle" option
because some dependencies break when mangled by babel-minify (which is a bit mysterious since it should
be able to handle them well, but there's at least one case where an eval() in a dependency breaks the supervisor).
This adds ~800kB to the resulting uncompressed image.

Since the optimization now happens with a plugin instead of the webpack command line,
we can't do the no-optimize trick we were doing in dindctl now - but it should
also be less important now that we don't mangle names.

Change-Type: patch
Signed-off-by: Pablo Carranza Velez <pablo@resin.io>
pcarranzav pushed a commit to balena-os/balena-supervisor that referenced this issue Nov 9, 2017
We've been using UglifyJS 0.4.6 (the webpack default) so far, but this doesn't support ES6 and some dependency
updates are starting to cause builds to break (e.g. request/request#2772, which also happens to break
my builds in the multicontainer branch).

Here we switch to the latest uglifyjs-webpack-plugin which is designed for ES2015 support.

Change-Type: patch
Signed-off-by: Pablo Carranza Velez <pablo@resin.io>
springmeyer pushed a commit to mapbox/mapbox-studio-classic that referenced this issue Jan 10, 2018
@koresar
Copy link

koresar commented May 23, 2018

I asked other people what do they think of request package versioning strategy.

There are only 10 responses. Please, do the same poll and you'll get the similar result.

image

My point is - it does not matter what is the most SemVer spec compliant way of versioning. People expect NPM modules to bump major version when they drop support of an old node.js.

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

No branches or pull requests