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

Move pify from devDependencies to dependencies #2614

Merged
merged 1 commit into from Jun 9, 2017

Conversation

3 participants
@m-allanson
Member

m-allanson commented Jun 9, 2017

Which issue, if any, is this issue related to?

#2607

@m-allanson

This comment has been minimized.

Member

m-allanson commented Jun 9, 2017

@hudochenkov Maybe this will work? I haven't tested it in all scenarios from your issue

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 9, 2017

Hm... why it's a devDependency if it's regular dependency?

const pify = require("pify")

I don't think we should remove it, because we need this dependency.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 9, 2017

@m-allanson Good spot. It looks like pify was accidentally added as a devDependency, rather than a dependency. This PR moves it into dependency.

@jeddy3

jeddy3 approved these changes Jun 9, 2017

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 9, 2017

Looks like I was looking in some other place while was writing a comment above. Sorry ¯\_(ツ)_/¯ I'll test this change in a few hours.

@hudochenkov

Yes, it fixes the problem in Node 4. Nice catch, @m-allanson!

@hudochenkov hudochenkov merged commit aee270e into master Jun 9, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 95.488%
Details

@hudochenkov hudochenkov deleted the pify-dev-dep branch Jun 9, 2017

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 9, 2017

I believe we shouldn't add anything to chagelog? Or should we? This problem was introduced in 7.10.0 in this commit. And as I understand it only affects npm@2 users.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 9, 2017

It's a fix of a regression, right? Best add it to the changelog so that it's discoverable if someone runs into the issue when using 7.10.0

  • Fixed: "Cannot find module 'pify'" regression in node@4 with npm@2.
@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 9, 2017

Thanks. Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment