Throw error if closing tag contains Muststache Statement #982
Conversation
|
|
||
| assert.throws(() => { | ||
| parse('\nbefore <div><span></span {{some-modifier}}></div>'); | ||
| }, 'Error: Invalid end tag `div` (on line 2). (closing tags cannot contain modifiers).'); |
There was a problem hiding this comment.
should error point into span instead of div?
There was a problem hiding this comment.
@lifeart Thanks for this! You're absolutely right. This comment made me realize my tests weren't actually testing correctly, which lead me down the path the determining how to solve this whole issue much more elegantly.
| @@ -148,6 +148,15 @@ export abstract class HandlebarsNodeVisitors extends Parser { | |||
| ); | |||
|
|
|||
| case TokenizerState.beforeAttributeName: | |||
| if (this.currentNode && this.currentNode.type === 'EndTag') { | |||
There was a problem hiding this comment.
I'm surprised we enter this state during a close tag 🤔
There was a problem hiding this comment.
I was surprised as well. This state is hit in this scenario: <div></div {{some-modifier}}>, and it was actually already throwing this error"Error: expected start tag" which is triggered in the this.currentStartTag call on line 160, but doesn't really describe the error accurately.
currentStartTag snippet:
get currentStartTag(): Tag<'StartTag'> {
let node = this.currentNode;
assert(node && node.type === 'StartTag', 'expected start tag');
return node as Tag<'StartTag'>;
}If you comment out this if check, two of the tests I added will fail, throwing the expected start tag error instead of the one added in this PR.
There was a problem hiding this comment.
OK, gotcha, thank you for explaining.
I believe what is going on is that the HTML spec requires the tagname immediately after the </.
See the spec info here:
https://html.spec.whatwg.org/multipage/parsing.html#end-tag-open-state
And our implementation here:
I think essentially we want an else to the if above that throws a useful error (RE: invalid end tag). I think that would look something like:
endTagOpen() {
let char = this.consume();
if (char === '@' || char === ':' || isAlpha(char)) {
this.transitionTo(TokenizerState.endTagName);
this.tagNameBuffer = '';
this.delegate.beginEndTag();
this.appendToTagName(char);
} else {
this.delegate.reportSyntaxError('invalid closing tag');
}
}Would you be interested in working on a fix over in simple-html-tokenizer also?
There was a problem hiding this comment.
I also think this is a bit wrong:
We have created the endTagName state, and made it essentially the same as the tagName state (the spec actually says to use the tagName state (see here), but thats a tad bit difficult for us). But honestly, I don't think we actually are allowed to enter those other states (beforeAttributeName and selfClosingStartTag). We should probably add errors there too.
There was a problem hiding this comment.
I'll make an attempt at fixing those issues in Simple HTML Tokenizer this weekend. I'll open a PR with the fixes, or an issue, if I'm unable to come up with solutions
Thanks for looking into this!
There was a problem hiding this comment.
Made a PR here tildeio/simple-html-tokenizer#73 would love feedback 😄
Edit: Changed entire description since it was no longer relevant.
This PR resolves #978 by Checking the state of the the tokenizer whenever it hits a
MustacheStatementand throws aSyntaxErrorif the tokenizer is in in anEndTagQuite open to feedback on this approach! As well as what the error message should be say exactly.