43081j / eslint-plugin-lit

lit-html support for ESLint
120 stars 22 forks source link

fix: take into account void elements and allow self-closing opening tags #68

Closed jorenbroekema closed 4 years ago

jorenbroekema commented 4 years ago

WIP: need to add the actual fix, this is just a failing test

Could use some guidance here on where I need to look for fixing this. I don't think parse5 contains information about whether an element is a void element that can contain a self-closing slash, at least I couldn't see it when checking it in ASTExplorer.

So feels like we need to grab a list (ideas welcome!) or maintain a list ourselves and add a fix to not error on the self-closing slash?

Let me know your thoughts, I'm happy to finish it up, fixes https://github.com/43081j/eslint-plugin-lit/issues/63 cc: @aress31

jorenbroekema commented 4 years ago

Seems like I was wrong, it's indeed in parse5 🤷‍♂️ kinda weird that in ASTExplorer they don't complain about it

43081j commented 4 years ago

yup sorry just catching up on this...

parse5 internally has a visitor method for each kind of tag effectively. some of these set a flag to specify that the tag can be self-closing, so this implicitly creates a "whitelist" of tags already.

its been a while since i investigated this problem but basically from what i remember, the error is thrown by parse5 and not us. still, it works in astexplorer (which uses some version of parse5) so is still worth debugging.

43081j commented 4 years ago

you can see here: https://github.com/inikulin/parse5/blob/252819607421a5741cf745bb60c404f023531b0d/packages/parse5/lib/parser/index.js#L1542-L1550

that it should in fact set ackSelfClosing which is what controls whether this error is thrown or not.

im afraid it'll just need some debugging/breakpoints to see what exactly parse5 is seeing when it throws the error, here: https://github.com/inikulin/parse5/blob/252819607421a5741cf745bb60c404f023531b0d/packages/parse5/lib/parser/index.js#L688

43081j commented 4 years ago

it is a bug in parse5!

https://github.com/inikulin/parse5/blob/252819607421a5741cf745bb60c404f023531b0d/packages/parse5/lib/parser/index.js#L1549

here p is the parser, it has no such property ackSelfClosing. it should read token.ackSelfClosing = true; instead.

i have no clue why astexplorer works but this is definitely wrong in parse5's code as far as i can see

edit:

i opened inikulin/parse5#321

jorenbroekema commented 4 years ago

Oooh good catch lol. I only got as far as ackSelfClosing not being true for <hr /> so it throws but I couldn't figure out what was going on, and then I got distracted by other things. Didn't notice it's token. for all instances but p. for the hr.

Thanks for looking into it even after my initial assumption was wrong :D interesting still that the ASTExplorer doesn't throw.

43081j commented 4 years ago

im gonna close this as the problem seems fixed by the new parse5 release (see #69 ).

thanks for helping out though, wouldn't have got fixed otherwise!