Skip to content
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

Adds SyntaxError #540

Merged
merged 1 commit into from
Jun 8, 2017
Merged

Adds SyntaxError #540

merged 1 commit into from
Jun 8, 2017

Conversation

twokul
Copy link
Contributor

@twokul twokul commented Jun 3, 2017

Adds a subclass of Error with additional location property. This helps to deliver error information to broccoli-builder and improve error reporting as described here.

I believe this closes #348

* Subclass of `Error` with additional information
* about location of incorrect markup.
*/
class SyntaxError extends Error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double check what this transpiles to? I vaguely recall issues with subclassing Error in IE9 and IE10. It may not matter as much here though since this code is primarily ran in node-land...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea this does not compile in Babel 6 correctly.

* about location of incorrect markup.
*/
class SyntaxError extends Error {
location: AST.SourceLocation;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we generally call this loc everywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loc is kind of ambiguous tbh, I'd prefer location (also the property name that broccoli-builder expects is location not loc). If you feel strongly that we should stay consistent here, I can rename.

@@ -149,7 +148,7 @@ export abstract class HandlebarsNodeVisitors extends Parser {
break;

default:
throw new Error(`Using a Handlebars comment when in the \`${tokenizer.state}\` state is not supported: "${comment.value}" on line ${loc.start.line}:${loc.start.column}`);
throw new SyntaxError(`Using a Handlebars comment when in the \`${tokenizer.state}\` state is not supported: "${comment.value}" on line ${loc.start.line}:${loc.start.column}`, rawComment.loc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should still include the loc info inside the message now that we thread loc through the rest of the system?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should; it makes the error message more useful and it would be weird to make it less useful imo

@rwjblue
Copy link
Member

rwjblue commented Jun 3, 2017

Overall, looks really good. Just had a few questions/clarifications....

@jfelchner
Copy link

This is going to be awesome! 👍

@rwjblue rwjblue merged commit c76176f into glimmerjs:master Jun 8, 2017
@twokul twokul deleted the errors branch June 9, 2017 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardize Template Error Structure
5 participants