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(cli): do not swallow any exceptions thrown by JS config files #140
Conversation
This was actually a bit more involved than I had anticipated--the |
I must also note there's something odd going on, because |
Oh, it's the snapshot stuff. Yeah, so that changed, because the only case in which you'd ever see that message is if the module couldn't be resolved by As a contributor, I'm not sure what I'm supposed to do about testing against snapshots I don't have on my local machine. IMO, this snapshot failure should be a warning, because otherwise it's crying wolf. Don't know if this is even possible with Travis CI. |
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.
Looks great! Thanks.
I think you need to update snapshots for the CLI tests. You can do this with: |
@kentcdodds Ahh, thanks. Didn't know they were in VCS. Updated & pusht |
A missing config file will report a warning, but any exception that the config file itself throws will be reported to the user. "Empty" configs are now also considered invalid; e.g. a 0-byte .js file or one exporting an plain empty object. 22
Codecov Report
@@ Coverage Diff @@
## master #140 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 10 10
Lines 308 315 +7
=====================================
+ Hits 308 315 +7
Continue to review full report at Codecov.
|
Some of the extra info from @codecov-io is cool, but this is a bit nonsensical: |
Yeah, it's more useful when you don't have 100% code coverage. I pretty much ignore the bot 🙃 thanks a bunch! |
A missing config file will report a warning, but any exception that the config file itself throws
will be reported to the user. "Empty" configs are now also considered invalid; e.g. a 0-byte .js
file or one exporting an plain empty object.
What: #22
Why: Provides a stack trace to user instead of "cannot find" warning, which is inaccurate
How: First leverage
require.resolve
to ensure the module exists and is readable. If not, warn. If so, execute. If an exception is thrown, report it.This also requires configs to be non-empty, as I was testing this by using
touch package-scripts.js
, which resulted in this lovely exception:Every module loaded by Node has an
exports
object; if you happen to have a 0-byte.js
file, theexports
object still exists, but there's nothing in it, and certainly not ascripts
object--thus, the above exception.With my changes, the above won't happen;
nps
will refer the user to this explanation, which has been updated to require non-empty configs.(I also note that
nps
should make a fuss if you are exporting an object, but said object lacks ascripts
property. This will likely result in a similar exception, and I have not addressed that case.I can also move this particular change into a new PR or commit if you wish)