-
-
Notifications
You must be signed in to change notification settings - Fork 425
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: Make app package.json load error tolerant #479
Conversation
efb16d7
to
5305cb1
Compare
Codecov Report
@@ Coverage Diff @@
## master #479 +/- ##
==========================================
- Coverage 98.74% 97.93% -0.82%
==========================================
Files 12 12
Lines 239 242 +3
Branches 28 28
==========================================
+ Hits 236 237 +1
- Misses 3 5 +2
Continue to review full report at Codecov.
|
let pkg | ||
try { | ||
// eslint-disable-next-line import/no-dynamic-require, global-require | ||
pkg = require(`${process.cwd()}/package.json`) |
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.
What is going to happen if I run commit from a sub-directory? The idea was that lint-staged
can still go up enough levels to locate the package.json
. AFAIK this won't work anymore from sub-dirs.
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.
We want to look at the package.json responsible for the pre-commit hook right? That is the same as process.cwd()
. In any case, like I mentioned, it won't break any valid config as we tolerate errors, if any, faced while loading 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.
Replied with more info in #477 (comment)
Commit from sub-directory works fine.
Sorry but I thought it's better to keep the discussion about this in the original PR since it brings more context. See #477 (comment) |
@okonet I won't be able to merge this because it brings down the coverage just a tiny bit. Could you please squash and merge? |
Done |
See #477 (comment)