Synchronously parse html / svg.

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.
Netflix · Dec 6, 2024 · c03cf79 · c03cf79
1 parent 1199e79
commit c03cf79
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 45 deletions.
diff --git 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
@@ -979,7 +979,7 @@ describe('changing content values', () => {
 describe('svg rendering', () => {
   it('renders a basic string', () => {
     const getTemplate = ({ r, cx, cy }) => {
-      return svg`<circle id="target" r="${r}" cx="${cx}" cy="${cy}"/>`;
+      return svg`<circle id="target" r="${r}" cx="${cx}" cy="${cy}"></circle>`;
     };
     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`
       <style>
         div { background-color: ${'red'}; }
       </style>
-    `);
+    `;
     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`
       <script id="target">
         console.log('${evil}');
       </script>
-    `);
+    `;
     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`<textarea id="target">please ${'foo'} no</textarea>`);
+    const callback = () => html`<textarea id="target">please ${'foo'} no</textarea>`;
     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`<textarea id="target"><b>please ${'foo'} no</b></textarea>`);
+    const callback = () => html`<textarea id="target"><b>please ${'foo'} no</b></textarea>`;
     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`<title>please ${'foo'} no</title>`);
+    const callback = () => html`<title>please ${'foo'} no</title>`;
     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`<title><b>please ${'foo'} no</b></title>`);
+    const callback = () => html`<title><b>please ${'foo'} no</b></title>`;
     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`<div id="target" not-ok=${'foo'}>Gotta double-quote those.</div>`);
+    const callback = () => html`<div id="target" not-ok=${'foo'}>Gotta double-quote those.</div>`;
     const expectedMessage = 'Found invalid template on or after line 1 in substring `<div id="target" not-ok=`. Failed to parse ` not-ok=`.';
     assertThrows(callback, expectedMessage);
   });
 
   it('throws for single-quoted attributes', () => {
-    const container = document.createElement('div');
-    const callback = () => render(container, html`\n<div id="target" not-ok='${'foo'}'>Gotta double-quote those.</div>`);
+    const callback = () => html`\n<div id="target" not-ok='${'foo'}'>Gotta double-quote those.</div>`;
     const expectedMessage = 'Found invalid template on or after line 2 in substring `\n<div id="target" not-ok=\'`. Failed to parse ` not-ok=\'`.';
     assertThrows(callback, expectedMessage);
   });
 
   it('throws for unquoted properties', () => {
-    const container = document.createElement('div');
-    const callback = () => render(container, html`\n\n\n<div id="target" .notOk=${'foo'}>Gotta double-quote those.</div>`);
+    const callback = () => html`\n\n\n<div id="target" .notOk=${'foo'}>Gotta double-quote those.</div>`;
     const expectedMessage = 'Found invalid template on or after line 4 in substring `\n\n\n<div id="target" .notOk=`. Failed to parse ` .notOk=`.';
     assertThrows(callback, expectedMessage);
   });
 
   it('throws for single-quoted properties', () => {
-    const container = document.createElement('div');
-    const callback = () => render(container, html`<div id="target" .notOk='${'foo'}'>Gotta double-quote those.</div>`);
+    const callback = () => html`<div id="target" .notOk='${'foo'}'>Gotta double-quote those.</div>`;
     const expectedMessage = 'Found invalid template on or after line 1 in substring `<div id="target" .notOk=\'`. Failed to parse ` .notOk=\'`.';
     assertThrows(callback, expectedMessage);
   });
@@ -1199,6 +1189,16 @@ describe('html errors', () => {
 //  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></-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></-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
diff --git 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) {