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

Remove unleash dependency #1522

Merged
merged 3 commits into from Oct 18, 2017

Conversation

boennemann
Copy link
Contributor

@boennemann boennemann commented Oct 18, 2017

Pre-Submission Checklist

Changes

With the release of 6.2.0 (09066d8) the unleash package got added as a dependency of this project. I suppose this happened accidentally, as it's not a real runtime dependency of this project.

Aside from downloading unnecessary bytes this has a very unfortunate consequence.
With the unleash package both "npm-check" and "npm" end up in the users node_modules/.bin folder.

Now if you run npx inside a project that uses restify, or npx npm-check -u for that matter, you end up with unleash's outdated npm-check version, and even worse with an npm@2.

This lead to verify confusing and seemingly unrelated error messages – until I could track it down to this slip-up in restify.

I hope my changes are in the right format, so you can merge this PR directly and release a new version.

I also removed lodash from devDependencies, because it's a dependency already. This gets rid of an npm warning.

Thanks a lot!

@EvgenyOrekhov
Copy link

I would call this a fix because unleash and npm@2 broke all my apps. See more info here: npm/npm#18874.

@retrohacker
Copy link
Member

Hey all! Sorry for this, looks like npm's new "default to --save" behavior caught one of our contributors by surprise. Thank you for catching this and filing an issue so quickly @boennemann! ❤️ sorry we broke your builds!

Also looks like we failed to semver bump our npm-shrinkwrap file, thank you for doing this in this PR.

@boennemann
Copy link
Contributor Author

@EvgenyOrekhov You're right. Updated the commit message.

@retrohacker Don't worry, mistakes happen :) In order to keep them from happening again I added another commit. It changes the unleash instructions to use npx. That way nobody has to install unleash explicitly.

@retrohacker
Copy link
Member

TIL: npx is a thing! That is awesome!

@retrohacker retrohacker merged commit a43aa60 into restify:master Oct 18, 2017
@boennemann boennemann deleted the remove-unleash-dependency branch October 18, 2017 14:59
@boennemann
Copy link
Contributor Author

🎉

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.

None yet

3 participants