-
Notifications
You must be signed in to change notification settings - Fork 14
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
Improve template issue forensics. #202
Conversation
@@ -142,7 +142,7 @@ it('errors are thrown in connectedCallback when template result fails to render | |||
try { | |||
el.connectedCallback(); | |||
} catch (error) { | |||
const expected = ' — Invalid template for "TestElement" at path "test-element-4[id="testing"][class="foo bar"][boolean][variation="primary"]"'; | |||
const expected = 'Invalid template for "TestElement" / <test-element-4> at path "test-element-4[id="testing"][class="foo bar"][boolean][variation="primary"]".'; |
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.
Seeing something like <test-element-4>
is often more useful than the actual class name for finding the file-in-question. I honestly don’t know why we hadn’t done this yet 🤔
test/test-template-engine.js
Outdated
@@ -884,7 +884,7 @@ describe('rendering errors', () => { | |||
} catch (e) { | |||
error = e; | |||
} | |||
assert(error?.message === `Found invalid template string "<div id="target" .notOk=" at " .notOk=".`, error.message); | |||
assert(error?.message === `Found invalid template at line 4 within string \`\n\n\n<div id="target" .notOk=\` at \` .notOk=\`.`, error.message); |
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.
The at line 4
text here is wildly helpful . It makes it exceedingly easy to track down template issues. IMO, if we’re gonna be strict about templates, we should strive to be increasingly specific about what went wrong and where.
@klebba — I was kicking the tires again on migrating to the default template engine. These changes make the process much more efficient. |
c5ae042
to
d8f5866
Compare
test/test-template-engine.js
Outdated
container.remove(); | ||
}); | ||
|
||
// Note that this is _still_ line “1” because it begins with a newline. This | ||
// is what authors will expect as the “first line”. |
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.
This is pretty nuanced, but I want the following to both conceptually report the “first line”:
html`<div .oops='${whoops}'></div>`;
html`
<div .oops='${whoops}'></div>
`;
… i.e., when the first line is literally just a newline and nothing else, ignore it, it’s not what I would consider the first line. The rational here is that if / when you could copy this text into a separate html file (not in our lifetimes), it would match the line numbering there.
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.
Interesting — IMO stripping the newlines is more confusing because the actual code is:
<div .oops='${whoops}'></div>
I might prefer to keep this precise vs smart, but I do not feel strongly about it
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.
stripping the newlines
🤔 — Hmm, definitely not stripping newlines! More like conditionally deleting the initial newline. I think you’d agree that in either case, the html file would look like this, right? I.e., the markup there is on line one. I can remove the fork though. Not worth arguing 🤙
<div></div>
x-element.js
Outdated
? handled.split('\n').length - 1 | ||
: handled.split('\n').length; | ||
// We say “on or after” because interpolated JS could add lines we | ||
// cannot know about or account for. |
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.
Note that you can write templates like this, hence the “on or after” phrase:
html`
<div .ok="${
'i can haz lines'
}" .oops='${whoops}'>
</div>
`;
This is gonna report like “on or after line 1” and there’s really not much we can do about that!
x-element.js
Outdated
@@ -1385,7 +1387,14 @@ class TemplateEngine { | |||
string = string.slice(0, -3 - name.length) + `${TemplateEngine.#ATTRIBUTE_PREFIXES.property}${iii}="${name}`; | |||
state.index = 1; // Accounts for an expected quote character next. | |||
} else { | |||
throw new Error(`Found invalid template string "${string}" at "${string.slice(state.index)}".`); | |||
// We try and intuit what authors consider line “1”. |
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.
Seeing the fork here does make me think that we should avoid exceptions like this — if the user chooses to author templates in a certain manner that will have implications for line numbering I would expect that the library should remain agnostic
d8f5866
to
43afbd4
Compare
The major changes can be boiled down into the following additions: * Print the registered custom element tag name in the error. * Print the approximate template line number in the error.† † Note that this isn’t the _file line number_, just a naive count of the number of newlines encountered so far in the given template. Closes #201.
43afbd4
to
3717cc0
Compare
@klebba — Gonna get this one merged in based on our conversation earlier 🤙 |
The major changes can be boiled down into the following additions:
† Note that this isn’t the file line number, just a naive count of
the number of newlines encountered so far in the given template.
Closes #201