-
-
Notifications
You must be signed in to change notification settings - Fork 422
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
fix: node-please-upgrade usage #575
Conversation
Codecov Report
@@ Coverage Diff @@
## master #575 +/- ##
=======================================
Coverage 98.14% 98.14%
=======================================
Files 13 13
Lines 378 378
Branches 52 52
=======================================
Hits 371 371
Misses 7 7 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s add the name to the object in the JS file to fix the output and leave package.json alone.
package.json
Outdated
@@ -10,6 +10,9 @@ | |||
"Suhas Karanth <sudo.suhas@gmail.com>" | |||
], | |||
"bin": "index.js", | |||
"engines": { | |||
"node": ">=8.6.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s was intentional not to have it here since it would break CIs with lower versions of Node installed without a particular reason. I’m on my mobile so I can’t search for the issue but please look for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, too bad we can't have optional dev deps 😞
We should at least pass the name to node-please-upgrade
to avoid broken message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that’s what I’m keen to merge. Could you please update the PR? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Before: `undefined requires at least version 8.6.0 of Node, please upgrade` After: `lint-staged requires at least version 8.6.0 of Node, please upgrade`
Hmm could you please check why is CI failing now? |
I think it's because I pushed too quickly and GitHub/Travis mixed things up a bit, because the tests passed: https://travis-ci.org/okonet/lint-staged/pull_requests |
Thanks for working on this! |
🎉 This PR is included in version 8.1.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Before:
undefined requires at least version 8.6.0 of Node, please upgrade
After:
lint-staged requires at least version 8.6.0 of Node, please upgrade