-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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: prevent crashing from JSON parsing error (fixes #10364) #10376
Fix: prevent crashing from JSON parsing error (fixes #10364) #10376
Conversation
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.
Thanks for contributing!
The pretty error messages only show up when using the command line interface directly (and not, for example, when using CLIEngine). So I think it could be valuable to leave the original error messages (that is, set both message
as well as messageTemplate
/data
)... What do you think?
So what should the |
I was thinking of the removal of line 117 in lib/config/config-file.js: we could leave that as is, but also set messageTemplate and data. Looking again, I don't see any other removals of |
The reason I removed that line is, IMHO, it's duplicated because the old line contains And, I haven't tested it by using Node.js API What do you think? |
@g-plane I think you're probably right that the message "Failed to parse JSON" is not super useful. But I also think "Syntax error: Unexpected token }" is not very useful either, hence the motivation for the message template that says failed to parse config file. Maybe we should still wrap/change If you get a chance to test this case in CLIEngine, hopefully you'll see what I'm getting at. 😄 |
Ok, you are right. Wrapping the error message is friendly for Another question,
Can we use |
Not sure what you mean. Could you please elaborate on this? Thanks! |
Here is a malformed json example: (missing comma at the end of line 2) {
"name": "test-eslint"
"version": "0.0.0",
"main": "index.js",
"license": "Apache-2.0",
"devDependencies": {
"eslint-config-gplane": "^3.0.0-beta.1"
}
} When using
That is the default message, but maybe we can use
But there is a problem that There is also an alternative to get better error. It's to use
|
@platinumazure I have updated for |
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.
This is looking really good! Just a few small typos.
Also: Could you add some assertions to some of the tests, to make sure messageTemplate/data.file/data.message are set correctly on the error objects? Thanks!
lib/config/config-file.js
Outdated
e.messageTemplate = "failed-to-parse-json"; | ||
e.messageData = { | ||
path: filePath, | ||
messsage: e.messsage |
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.
s/messsage/message (twice)
lib/util/npm-util.js
Outdated
e.messageTemplate = "failed-to-parse-json"; | ||
e.messageData = { | ||
path: pkgJson, | ||
messsage: e.messsage |
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.
s/messsage/message (twice)
It seems that there are no such tests to check message template. (Not only this template but also other templates.) |
Hi, apologies, I just meant adding any assertions to existing tests that
check that the exception is thrown in the first place. On top of checking
that the exception is thrown, we could inspect its properties.
We don't need to test the rendering of the template. We just want to test
that this exception is associated with a template... Hope that makes sense.
…On Sat, May 19, 2018, 11:12 Pig Fang ***@***.***> wrote:
It seems that there no such tests to check message template. (Not only
this template but also other templates.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10376 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AARWesYakvF3O2ujuDykmyP5YLUsNcKlks5t0ESIgaJpZM4UEOs9>
.
|
Updated. |
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.
LGTM, thanks for adding those tests/assertions! Would like another set of eyes on this before merging.
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.
Thanks for the PR! I just have a minor comment about improving the error message, but this looks good aside from that.
lib/config/config-file.js
Outdated
@@ -115,6 +115,11 @@ function loadJSONConfigFile(filePath) { | |||
} catch (e) { | |||
debug(`Error reading JSON file: ${filePath}`); | |||
e.message = `Cannot read config file: ${filePath}\nError: ${e.message}`; | |||
e.messageTemplate = "failed-to-parse-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.
I don't think the error here will necessarily be a JSON parsing error. This error-handling code will run when anything in JSON.parse(stripComments(readFile(filePath)))
throws an error, which could include errors from the filesystem.
This could either be addressed by using different try
/catch
blocks for the different function calls (and providing a different error message as appropriate), or making the failed-to-parse-json
error message more generic (e.g. "Failed to read JSON file" rather than "Failed to parse JSON file").
@not-an-aardvark I have moved it to a separated try/catch block. |
Thanks! However, I think the same concern about the error message applies to all of the other |
@not-an-aardvark Review again? |
Ping @not-an-aardvark: Have your concerns been addressed? |
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.
LGTM, thanks! Sorry about the delay in re-reviewing.
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
close #10364
What changes did you make? (Give an overview)
Display better error message when failed to parse JSON.