-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Add location information to parsing errors #7314
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6791/ |
@@ -110,6 +109,7 @@ function parser(pluginPasses, options, code) { | |||
err.message = | |||
`${options.filename || "unknown"}: ${err.message}\n\n` + codeFrame; | |||
} | |||
err.code = "BABEL_PARSE_ERROR"; |
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 know if this is the first time we started doing this but are we going to have some kind of convention for this, docs? Or is this just a one-off @loganfsmyth
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 think error codes are good. Probably in the long run anywhere we throw an error in babel-core should have its own code. Even better would be if Babylon already included a code and this code checked
if (err.code === "BABYLON_PARSE_ERROR")
then Babel wrapped it somehow, but eh :)
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.
Ok yeah ^ was what I was thinking. maybe @kaicataldo can you make an issue about that? I think a task could be to look through all the places we throw and to think about if we want to make an errors file kind of deal, etc. And I think we would want this to be consistent moving forward and to be more maintainable?
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.
Would be great to make this consistent! Would this be just for Babylon and babel-core? Or all packages?
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's funny also because I'm reminded we have a package called babel-messages
that we sometimes use?
packages/babel-template/src/parse.js
Outdated
err.message += "\n" + codeFrameColumns(code, { start: loc }); | ||
err.code = "BABEL_PARSE_ERROR"; |
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.
Since this isn't the user's own code failing parsing, I'd vote for this having a different code like BABEL_TEMPLATE_PARSE_ERROR
or something.
2337b7c
to
0056fdb
Compare
Updated! |
This reverts commit 7234442.
Whoops - was trying to delete the branch, not revert the merge! |
As discussed in Slack, this PR retains location information for errors thrown by the parser and adds error codes so that the consumer can distinguish between errors easily.
A few questions I have: