Improve parser error messaging and factoring. #283
Conversation
Taking on some of the language tweaks from #248. Also `switch`-ing on the values (versus using a `Map`) so that these cases can be enforced by code coverage. We likely have a couple messages which are impossible to get to. It’d be good to be forced to either remove them or find a way to prove that they are reachable via our test suite. For now, I reduced the coverage target to keep this change set smaller, but I will address that in follow-up efforts.
| @@ -291,6 +291,49 @@ html`<div>${fragment}</div>`; | |||
| // Error: Unexpected child element count of zero for given DocumentFragment. | |||
| ``` | |||
|
|
|||
| ## Supported native tags | |||
There was a problem hiding this comment.
As requested, added a section for #supported-native-tags in our TEMPLATES.md file (so that we can link to it from our error messaging).
| order to reduce complexity and improve performance. The following tags are | ||
| supported: | ||
|
|
||
| ```html |
There was a problem hiding this comment.
Thought it would be nice to present this with html since that’s what it’s about. Open to different formatting options, but this felt like it would get the point across in a concise way.
| @@ -7,7 +7,7 @@ import '../x-template.js'; | |||
|
|
|||
| // Set a high bar for code coverage! | |||
| coverage(new URL('../x-element.js', import.meta.url).href, 100); | |||
| coverage(new URL('../x-parser.js', import.meta.url).href, 100); | |||
| coverage(new URL('../x-parser.js', import.meta.url).href, 97); // TODO: Get back to 100%! | |||
There was a problem hiding this comment.
Will fix this later (pinky promise).
| @@ -699,6 +699,42 @@ describe('odds and ends', () => { | |||
| }); | |||
|
|
|||
| describe('errors', () => { | |||
| it('throws when initial markup cannot be parsed', () => { | |||
There was a problem hiding this comment.
Here are a couple cases I was able to add to improve our coverage.
| ['#157', 'Forbidden declarative shadow root used (e.g., `<template shadowrootmode="open">`).'], | ||
| ]); | ||
| static #getErrorMessage(key) { | ||
| switch (key) { |
There was a problem hiding this comment.
If you switch on this stuff, this will all get enforced by our code coverage targets. I had initially started trying to duplicate this mapping and enforce it in our tests… but that’s brittle. It’s really nice that this will just always be enforced moving forward.
| static #getErrorMessage(key) { | ||
| switch (key) { | ||
| case '#100': return 'Could not parse template markup (at template start).'; | ||
| case '#101': return 'Could not parse template markup (after text content).'; |
There was a problem hiding this comment.
Took on most of the language tweaks. Thanks @klebba!
| @@ -519,15 +532,15 @@ export class XParser { | |||
| const { parsed, notParsed } = XParser.#getErrorInfo(strings, stringsIndex, string, stringIndex); | |||
| const valueForbidden = XParser.#forbiddenTransition(string, stringIndex, value); | |||
| const valueMalformed = XParser.#invalidTransition(string, stringIndex, value); | |||
| const errorMessagesKey = valueForbidden | |||
| ? XParser.#valueForbiddenToErrorMessagesKey.get(valueForbidden) | |||
| const errorMessageKey = valueForbidden | |||
There was a problem hiding this comment.
Refactored some of the variable names to match the new setup. Unfortunately, that made this diff a little extra ugly.
Taking on some of the language tweaks from #248. Also
switch-ing on the values (versus using aMap) so that these cases can be enforced by code coverage.We likely have a couple messages which are impossible to get to. It’d be good to be forced to either remove them or find a way to prove that they are reachable via our test suite.
For now, I reduced the coverage target to keep this change set smaller, but I will address that in follow-up efforts.