-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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: handle case of invalid package.json with no explicit config #5198
base: main
Are you sure you want to change the base?
Conversation
Since `readFileSync` is only stubbed `onFirstCall` we get a different answer the second time around which is probably Not What You Want. But also we *already checked that config = false*. So you could just remove this test, it does nothing useful.
(signed the CLA!) |
lib/cli/options.js
Outdated
@@ -195,6 +195,11 @@ const loadPkgRc = (args = {}) => { | |||
`Unable to read/parse ${filepath}: ${err}`, | |||
filepath | |||
); | |||
} else if (err.toString().includes('SyntaxError')) { | |||
throw createUnparsableFileError( |
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.
(On my phone) Will throwing here result in a warning? Feels more like an error to me?
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.
It will fail with an error, these exceptions are not caught (we could, possibly, catch them at the top level of the CLI and exit cleanly in order to avoid barfing stack traces on the user...)
lib/cli/options.js
Outdated
@@ -195,6 +195,11 @@ const loadPkgRc = (args = {}) => { | |||
`Unable to read/parse ${filepath}: ${err}`, | |||
filepath | |||
); | |||
} else if (err.toString().includes('SyntaxError')) { |
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.
If we want to separate the parsing from the reading errors, then I would suggest instead splitting this try/catch into an initial one for reading followed by a second one for parsing – that way we will not have to check the error as we know it's JSON.parse()
that fails
Also gives more granular error for the existing use case – different error message depending on whether it can't read or can't parse
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.
Seems like a good idea. Also, would it be useful to give an error for parsing errors more like the one npm gives, i.e. "make sure your package.json is JSON and not Javascript"?
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 don’t think it’s needed, just a warning saying “could not check package.json due to error while parsing” is probably the clearest
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've simply split it into "Unable to read" and "Unable to parse" for the moment, not sure if there is a better wording.
Thanks for the PR! Gave some feedback |
PR Checklist
package.json
section #5141status: accepting prs
Overview
Handle the corner case which can happen if:
package.json
which npm was happy withmocha
configuration inside yourpackage.json
npm test
you ranmocha
directly (withnpx
or justmocha.js
)As detailed here: https://github.com/dhdaines/mocha-5141