From c03cf7994471c28793c0980231d55e68f2cdbff4 Mon Sep 17 00:00:00 2001 From: Andrew Seier Date: Thu, 5 Dec 2024 13:54:41 -0800 Subject: [PATCH] Synchronously parse html / svg. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the parsing only happened after calling “render”. The issue with this is that you lose the original line numbers on errors when something goes wrong. By forcing the parse to happen immediately, you can find the actual invocation of “html” within which the issue lies. The only potential downside to this approach would be a performance hit within our “update” path. This is because we would check whether the “strings” from our tagged template function have been analyzed or not. Checking for their existence in our WeakMap takes some non-zero amount of time. We’re going to go ahead and _try_ this approach and weight the costs against the benefits — it’s easy to revert. --- CHANGELOG.md | 2 ++ test/test-template-engine.js | 46 ++++++++++++++++++------------------ ts/x-template.d.ts.map | 2 +- x-template.js | 45 +++++++++++++++++++---------------- 4 files changed, 50 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f06601b..ecb3b4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 bloat the default template engine interface (#207, #216). - Pull “x-template.js” into a separate file. Conceptually it solves a totally different problem than “x-element” (#226). +- Throw immediately with parsing errors in default template engine. This is done + as an improvement to developer feedback (#233). ### Deprecated diff --git a/test/test-template-engine.js b/test/test-template-engine.js index 88556ee..a4bd914 100644 --- a/test/test-template-engine.js +++ b/test/test-template-engine.js @@ -979,7 +979,7 @@ describe('changing content values', () => { describe('svg rendering', () => { it('renders a basic string', () => { const getTemplate = ({ r, cx, cy }) => { - return svg``; + return svg``; }; const container = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); container.setAttribute('viewBox', '0 0 100 100'); @@ -1094,80 +1094,70 @@ describe('value issues', () => { describe('html errors', () => { it('throws when attempting to interpolate within a style tag', () => { - const container = document.createElement('div'); - const callback = () => render(container, html` + const callback = () => html` - `); + `; const expectedMessage = 'Interpolation of "style" tags is not allowed.'; assertThrows(callback, expectedMessage); }); it('throws when attempting to interpolate within a script tag', () => { const evil = '\' + prompt(\'evil\') + \''; - const container = document.createElement('div'); - const callback = () => render(container, html` + const callback = () => html` - `); + `; const expectedMessage = 'Interpolation of "script" tags is not allowed.'; assertThrows(callback, expectedMessage); }); it('throws when attempting non-trivial interpolation of a textarea tag', () => { - const container = document.createElement('div'); - const callback = () => render(container, html``); + const callback = () => html``; const expectedMessage = 'Only basic interpolation of "textarea" tags is allowed.'; assertThrows(callback, expectedMessage); }); it('throws when attempting non-trivial interpolation of a textarea tag via nesting', () => { - const container = document.createElement('div'); - const callback = () => render(container, html``); + const callback = () => html``; const expectedMessage = 'Only basic interpolation of "textarea" tags is allowed.'; assertThrows(callback, expectedMessage); }); it('throws when attempting non-trivial interpolation of a title tag', () => { - const container = document.createElement('div'); - const callback = () => render(container, html`please ${'foo'} no`); + const callback = () => html`please ${'foo'} no`; const expectedMessage = 'Only basic interpolation of "title" tags is allowed.'; assertThrows(callback, expectedMessage); }); it('throws when attempting non-trivial interpolation of a title tag via nesting', () => { - const container = document.createElement('div'); - const callback = () => render(container, html`<b>please ${'foo'} no</b>`); + const callback = () => html`<b>please ${'foo'} no</b>`; const expectedMessage = 'Only basic interpolation of "title" tags is allowed.'; assertThrows(callback, expectedMessage); }); it('throws for unquoted attributes', () => { - const container = document.createElement('div'); - const callback = () => render(container, html`
Gotta double-quote those.
`); + const callback = () => html`
Gotta double-quote those.
`; const expectedMessage = 'Found invalid template on or after line 1 in substring `
{ - const container = document.createElement('div'); - const callback = () => render(container, html`\n
Gotta double-quote those.
`); + const callback = () => html`\n
Gotta double-quote those.
`; const expectedMessage = 'Found invalid template on or after line 2 in substring `\n
{ - const container = document.createElement('div'); - const callback = () => render(container, html`\n\n\n
Gotta double-quote those.
`); + const callback = () => html`\n\n\n
Gotta double-quote those.
`; const expectedMessage = 'Found invalid template on or after line 4 in substring `\n\n\n
{ - const container = document.createElement('div'); - const callback = () => render(container, html`
Gotta double-quote those.
`); + const callback = () => html`
Gotta double-quote those.
`; const expectedMessage = 'Found invalid template on or after line 1 in substring `
{ // developer feedback in the future if the performance and complexity costs // aren’t too high. describe.todo('future html errors', () => { + it('throws every time if there is a parsing error', () => { + // At one point, we only threw the _first_ time we encountered a given + // tagged template function “strings” array. We want to throw always. + const callback = () => html`<-div>`; + const expectedMessage = 'TODO — Write a better error message!'; + assertThrows(callback, expectedMessage); + assertThrows(callback, expectedMessage); + assertThrows(callback, expectedMessage); + }); + it('throws if open tag starts with a hyphen', () => { const callback = () => html`<-div>`; const expectedMessage = 'TODO — Write a better error message!'; diff --git a/ts/x-template.d.ts.map b/ts/x-template.d.ts.map index c37b760..fdf5606 100644 --- a/ts/x-template.d.ts.map +++ b/ts/x-template.d.ts.map @@ -1 +1 @@ -{"version":3,"file":"x-template.d.ts","sourceRoot":"","sources":["../x-template.js"],"names":[],"mappings":"AA8gCA,yBAA2E;AAC3E,uBAAuE;AACvE,sBAAqE;AAGrE,sBAAqE;AACrE,uBAAuE;AACvE,6BAAmF;AACnF,4BAAiF;AACjF,4BAAiF;AACjF,0BAA6E;AAC7E,yBAA2E;AAG3E,8BAAqF;AACrF,+BAAuF;AACvF,wBAAyE;AACzE,2BAA+E;AAC/E,4BAAiF;AACjF,wBAAyE;AACzE,2BAA+E;AAC/E,kCAA6F;AAC7F,wBAAyE"} \ No newline at end of file +{"version":3,"file":"x-template.d.ts","sourceRoot":"","sources":["../x-template.js"],"names":[],"mappings":"AAihCA,yBAA2E;AAC3E,uBAAuE;AACvE,sBAAqE;AAGrE,sBAAqE;AACrE,uBAAuE;AACvE,6BAAmF;AACnF,4BAAiF;AACjF,4BAAiF;AACjF,0BAA6E;AAC7E,yBAA2E;AAG3E,8BAAqF;AACrF,+BAAuF;AACvF,wBAAyE;AACzE,2BAA+E;AAC/E,4BAAiF;AACjF,wBAAyE;AACzE,2BAA+E;AAC/E,kCAA6F;AAC7F,wBAAyE"} \ No newline at end of file diff --git a/x-template.js b/x-template.js index c30381b..c8833e6 100644 --- a/x-template.js +++ b/x-template.js @@ -20,10 +20,13 @@ class TemplateEngine { // Sentinel to hold raw result language. Also leveraged to determine whether a // value is a raw result or not. Template engine supports html and svg. - static #LANGUAGE = Symbol(); static #HTML = 'html'; static #SVG = 'svg'; + // Sentinel to hold internal result information. Also leveraged to determine + // whether a value is a raw result or not. + static #ANALYSIS = Symbol(); + // Sentinel to initialize the “last values” array. static #UNSET = Symbol(); @@ -834,21 +837,11 @@ class TemplateEngine { } } - // Inject a given result into a node for the first time. If we’ve never seen - // the template “strings” before, we also have to generate html, parse it, - // and find out binding targets. Then, we commit the values by iterating over - // our targets. Finally, we actually attach our new DOM into our node. + // Inject a given result into a node for the first time. static #inject(rawResult, node, before) { - // Create and prepare a document fragment to be injected. - const { [TemplateEngine.#LANGUAGE]: language, strings } = rawResult; - const analysis = TemplateEngine.#setIfMissing(TemplateEngine.#stringsToAnalysis, strings, () => ({})); - if (!analysis.done) { - analysis.done = true; - const fragment = TemplateEngine.#createFragment(language, strings); - const lookups = TemplateEngine.#findLookups(fragment); - analysis.fragment = fragment; - analysis.lookups = lookups; - } + // Get fragment created from a tagged template function’s “strings”. + const { [TemplateEngine.#ANALYSIS]: analysis } = rawResult; + const language = analysis.language; const fragment = analysis.fragment.cloneNode(true); const targets = TemplateEngine.#findTargets(fragment, analysis.lookups); const preparedResult = { rawResult, fragment, targets }; @@ -875,11 +868,22 @@ class TemplateEngine { } static #createRawResult(language, strings, values) { - return { [TemplateEngine.#LANGUAGE]: language, strings, values }; + const analysis = TemplateEngine.#setIfMissing(TemplateEngine.#stringsToAnalysis, strings, () => ({})); + if (!analysis.done) { + const fragment = TemplateEngine.#createFragment(language, strings); + const lookups = TemplateEngine.#findLookups(fragment); + analysis.language = language; + analysis.fragment = fragment; + analysis.lookups = lookups; + analysis.done = true; + } + // This is a leaking implementation detail, but fixing the leak comes at + // a non-negligible performance cost. + return { [TemplateEngine.#ANALYSIS]: analysis, strings, values }; } static #isRawResult(value) { - return !!value?.[TemplateEngine.#LANGUAGE]; + return !!value?.[TemplateEngine.#ANALYSIS]; } // TODO: Revisit this concept when we delete deprecated interfaces. Once that @@ -933,10 +937,9 @@ class TemplateEngine { } static #canReuseDom(preparedResult, rawResult) { - return ( - preparedResult?.rawResult[TemplateEngine.#LANGUAGE] === rawResult?.[TemplateEngine.#LANGUAGE] && - preparedResult?.rawResult.strings === rawResult?.strings - ); + // TODO: Is it possible that we might have the same strings from a different + // template language? Probably not. The following check should suffice. + return preparedResult?.rawResult.strings === rawResult?.strings; } static #createCursors(referenceNode) {