cthackers / adm-zip

A Javascript implementation of zip for nodejs. Allows user to create or extract zip files both in memory or to/from disk
MIT License
2.06k stars 374 forks source link

adm-zip@0.4.5 breaks yeoman-generator on node v0.12.0 and iojs #121

Closed rmg closed 9 years ago

rmg commented 9 years ago

v0.4.5 introduced a dependency on fidonet-mailer-binkp-crypt which does not build with node v0.12.0 or any version of iojs. See askovpen/node-fidonet-mailer-binkp-crypt#1

Unfortunately, adm-zip is a dependency of decompress-unzip, which is a dependency of download, which is a dependency of yeoman-generator, so I think this actually broke most of the yeoman ecosystem for anyone not using node v0.10.x.

kud commented 9 years ago

It's not only yeoman. It's also napa and gulp-imagemin for example.

chetankumar01 commented 9 years ago

and it's also broken for 0.10.36

pastur commented 9 years ago

and 0.10.33

dcopestake commented 9 years ago

178 dependents listed on npmjs, so the impact of this is pretty big. I came across it because of a dependency on grunt-contrib-imagemin which eventually ends up here.

cgadam commented 9 years ago

and 0.11.14

ocombe commented 9 years ago

That's why you never add new functionalities as a patch but as a minor bump. Adm-zip should have bumped the version to 0.5.0 instead of 0.4.5... Semver is here for a reason :-/

In the mean time I set the version of adm-zip in my package.json and I can now install imagemin without any problem: npm install --save-dev adm-zip@0.4.4

kud commented 9 years ago

+1 @ocombe

rmg commented 9 years ago

Depending on which maintainer sees it first, this may end up being fixed by askovpen/node-fidonet-mailer-binkp-crypt#2

dcopestake commented 9 years ago

I think even if gets fixed in the dependency, as @ocombe pointed out, the reason this broke for me was because of the new functionality/dependency being included in a minor version update. I'm not sure whether 0.4.6 could be released which reverts this back to 0.4.4 and then 0.5.0 could be re-released with the fixed dependency?

hudsonhebert commented 9 years ago

It worked for me (use @ instead of #): npm install --save-dev adm-zip@0.4.4

ocombe commented 9 years ago

Yes it's an arobase thanks, I made the change in my package.json directly :)

cgadam commented 9 years ago

Looks like it is fixed now thanks to the pull mentioned by @rmg -> askovpen/node-fidonet-mailer-binkp-crypt#2

misterbrownlee commented 9 years ago

Heads up that "fidonet-mailer-binkp-crypt": ">0.0.20" as your dependency is a really bad idea. You are trusting @askovpen with the stability of your whole project.

Pro-tip: DO NOT USE ~, >, or really even ^ if you want to have production worthy library. That goes double if you're depending on a 0.x.x version.

I'd link to the definition of SemVer here, but I think we all know what version 0 means.

BTW, I don't see everything as fine yet, because fidonet-mailer-binkp-crypt is still breaking on our Linux CI server. See this issue: https://github.com/karma-runner/karma-sauce-launcher/issues/65

lazd commented 9 years ago

+1 to what @tehfoo said, "fidonet-mailer-binkp-crypt": ">0.0.20" is irresponsible, especially so considering it has dependencies like "bindings": ">0.0.0".

PLEASE pay attention to semver, people.

misterbrownlee commented 9 years ago

Further heads up that you might not be fully out of the woods yet: https://github.com/cthackers/adm-zip/issues/123

cthackers commented 9 years ago

ugh, this exploded a bit. i removed the crypt thing and pushed a new release on npm.

rmg commented 9 years ago

I wouldn't normally consider this a change worthy of bumping the major version of adm-zip, but I just noticed in the README that this module's raison d'être is that it is pure JS and has no dependencies.

@cthackers thanks for pulling the dep. Is there anything anyone could help with in the form of a PR?

cthackers commented 9 years ago

I didn't bump the major version, it was 0.4.4 and i made it 0.4.5 :) @rmg, what do you mean with the PR ?

rmg commented 9 years ago

@cthackers I meant I didn't think your decision to do a patch bump was entirely wrong. Probably should have been a minor, at least, since it added a couple functions but was otherwise backwards compatible. But then I noticed the mention in the readme about being pure js, which means you broke that "part" of the API.

re: PR, I mean this project just got a whole lot of attention, making it pretty clear how important this module is. Is there anything you would like to ask for help with that people could submit pull requests for?

cthackers commented 9 years ago

Why in the world did it get so much attention ? Is there something I'm not aware of ? It's been here for some years now.

misterbrownlee commented 9 years ago

Your use of > was part of the problem. You basically trusted any release of fidonet-mailer-binkp-crypt, and he pushed a "dot" release that had a major system dependency change.

:bomb: :crying_cat_face:

For us, the attention came because you are downstream via karma > karma-sauce-connect. I had 60 jobs breaking on npm install. Thanks for the quick update.

cthackers commented 9 years ago

Well, now is fixed. Thanks all.

rmg commented 9 years ago

@tehfoo the version specifier wasn't an issue because that module hadn't been updated in 8 months (until this whole ordeal). The issue is adm-zip added it as a dependency when it wasn't one before, and meant a compiler was now required, starting with the 0.4.5 release made last night. The fact that fidonet-mailer-binkp-crypt happened to have compilation errors on many versions of node was just the symptom everyone saw.

ocombe commented 9 years ago

Anyway thanks for fixing it fast @cthackers and I think that there is a good lesson learned here for all of us!

kud commented 9 years ago

The more I know node modules, the more I think fixed versions are the best. No ~, no ^, or >. People do not know how to use semver. What's a shame but I have, we have to deal with it.

clmath commented 9 years ago

@cthackers Thanks for your quick fix :thumbsup: