Adding a couple minor changes for 2.x interface.

theengineear · theengineear · commit f920e8088927 · 2024-11-16T10:40:20.000-08:00
Changes:
* Stricter handling of “textarea” binding. Previously we were overly
  lenient in how the binding worked which caused automagical behavior.
  For example, newlines were quietly ignored, which could lead to issues
  if you really _wanted_ newlines in a default value in a text area
  control and were actually trying to interpolate between lines. Now, it
  just throws for that case.
* Spec-compliant attribute traversal. Previously, we relied on the
  browser convention that attributes in a NamedNodeMap would be iterated
  over in a particular manner. The spec strictly indicates that this is
  not to be relied on — so we can oblige.
* The deprecated `plaintext` html tag is no longer considered. Use at
  your own peril…
* General improvements to inline documentation and naming of things.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,13 +20,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The `ifDefined` updater now deletes the attribute on `null` in addition to
   `undefined`. This makes it behave identically to `nullish`. However, both
   updaters are deprecated and the `??attr` binding should be used instead.
+- Interpolation of `textarea` is stricter. This used to be handled with some
+  leniency — `<textarea>\n ${value} \n</textarea>`. Now, you have to fit the
+  interpolation exactly — `<textarea></textarea>`.
 
 ### Deprecated
 
 - The `ifDefined` and `nullish` updaters are deprecated, update templates to use
   syntax like `??foo="${bar}"`.
 - The `repeat` updater is deprecated, use `map` instead.
 - The `unsafeHTML` and `unsafeSVG` updaters are deprecated, use `unsafe`.
+- The `plaintext` tag is no longer handled. This is a deprecated html tag which
+  required special handling… but it’s unlikely that anyone is using that.
 
 ### Fixed
 
diff --git a/test/test-template-engine.js b/test/test-template-engine.js
@@ -65,6 +65,75 @@ describe('html rendering', () => {
     container.remove();
   });
 
+  // Unlike a NodeList, a NamedNodeMap (i.e., “.attributes”) is not technically
+  //  ordered in any way. This test just confirms that the template engine logic
+  //  doesn’t get confused in any way post-parse.
+  it('renders elements with many attributes in a weird order', () => {
+    const getTemplate = ({
+      property1,
+      z99,
+      defined,
+      dataFoo,
+      property2,
+      title,
+      dataBar,
+      className,
+      property3,
+      boolean,
+      ariaLabel,
+      content,
+    }) => {
+      return html`
+        <div
+          id="target"
+          .property1="${property1}"
+          z-99="${z99}"
+          ??defined="${defined}"
+          data-foo="${dataFoo}"
+          .property2="${property2}"
+          title="${title}"
+          static="just hanging"
+          data-bar="${dataBar}"
+          class="${className}"
+          .property3="${property3}"
+          ?boolean="${boolean}"
+          aria-label="${ariaLabel}">
+          ${content}
+        </div>
+      `;
+    };
+    const container = document.createElement('div');
+    document.body.append(container);
+    render(container, getTemplate({
+      property1: null,
+      z99: true,
+      defined: false,
+      dataFoo: 10,
+      property2: -1,
+      title: 'a title',
+      dataBar: 'data attribute',
+      className: 'a b c',
+      property3: new URL('https://github.com/Netflix/x-element'),
+      boolean: 'yes',
+      ariaLabel: 'this is what it does',
+      content: 'influencing',
+    }));
+    const target = container.querySelector('#target');
+    assert(target.property1 === null);
+    assert(target.getAttribute('z-99') === 'true');
+    assert(target.getAttribute('defined') === 'false');
+    assert(target.getAttribute('data-foo') === '10');
+    assert(target.property2 === -1);
+    assert(target.getAttribute('title') === 'a title');
+    assert(target.getAttribute('data-bar') === 'data attribute');
+    assert(target.getAttribute('class') === 'a b c');
+    assert(target.property3.href === 'https://github.com/Netflix/x-element');
+    assert(target.getAttribute('boolean') === '');
+    assert(target.getAttribute('aria-label') === 'this is what it does');
+    assert(target.textContent.trim() === 'influencing');
+    container.remove();
+  });
+
   it('renders multiple, interpolated content', () => {
     const getTemplate = ({ one, two }) => {
       return html`
@@ -1058,6 +1127,38 @@ describe('rendering errors', () => {
       container.remove();
     });
 
+    it('throws when attempting non-trivial interpolation of a textarea tag', () => {
+      const getTemplate = ({ content }) => {
+        return html`<textarea id="target">please ${content} no</textarea>`;
+      };
+      const container = document.createElement('div');
+      document.body.append(container);
+      let error;
+      try {
+        render(container, getTemplate({ content: 'foo' }));
+      } catch (e) {
+        error = e;
+      }
+      assert(error?.message === `Only basic interpolation of "textarea" tags is allowed.`, error.message);
+      container.remove();
+    });
+
+    it('throws when attempting non-trivial interpolation of a title tag', () => {
+      const getTemplate = ({ content }) => {
+        return html`<title id="target">please ${content} no</title>`;
+      };
+      const container = document.createElement('div');
+      document.body.append(container);
+      let error;
+      try {
+        render(container, getTemplate({ defaultValue: 'foo' }));
+      } catch (e) {
+        error = e;
+      }
+      assert(error?.message === `Only basic interpolation of "title" tags is allowed.`, error.message);
+      container.remove();
+    });
+
     it('throws for unquoted attributes', () => {
       const templateResultReference = html`<div id="target" not-ok=${'foo'}>Gotta double-quote those.</div>`;
       const container = document.createElement('div');
diff --git a/x-element.js b/x-element.js
@@ -1018,6 +1018,7 @@ class TemplateEngine {
   static #DEFINED_PREFIX = 'x-element-defined';
   static #PROPERTY_PREFIX = 'x-element-property';
   static #CONTENT_PREFIX = 'x-element-content';
+  static #ATTRIBUTE_PADDING = 6;
 
   // Patterns to find special edges in original html strings.
   static #OPEN_REGEX = /<[a-z][a-z0-9-]*(?=\s)/g;
@@ -1475,6 +1476,9 @@ class TemplateEngine {
     TemplateEngine.#mapInner(node, startNode, identify, callback, value, 'repeat');
   }
 
+  // Walk through each string from our tagged template function “strings” array
+  //  in a stateful way so that we know what kind of bindings are implied at
+  //  each interpolated value.
   static #exhaustString(string, state) {
     if (!state.inside) {
       // We're outside the opening tag.
@@ -1502,6 +1506,29 @@ class TemplateEngine {
     }
   }
 
+  // Flesh out an html string from our tagged template function “strings” array
+  //  and add special markers that we can detect later, after instantiation.
+  //
+  // E.g., the user might have passed this interpolation:
+  //
+  // <div
+  //   id="foo-bar-baz"
+  //   foo="${foo}"
+  //   bar="${bar}"
+  //   .baz="${baz}">
+  //   ${content}
+  // </div>
+  //
+  // … and we would instrument it as follows:
+  //
+  // <div
+  //   id="foo-bar-baz"
+  //   x-element-attribute-000001="foo"
+  //   x-element-attribute-000002="bar"
+  //   x-element-property-000003="baz">
+  //   <!--x-element-content-->
+  // </div>
+  //
   static #createHtml(type, strings) {
     const htmlStrings = [];
     const state = { inside: false, index: 0 };
@@ -1527,13 +1554,15 @@ class TemplateEngine {
               case '??': prefix = TemplateEngine.#DEFINED_PREFIX; syntax = 4; break;
               case '?': prefix = TemplateEngine.#BOOLEAN_PREFIX; syntax = 3; break;
             }
-            string = string.slice(0, -syntax - attribute.length) + `${prefix}-${iii}="${attribute}`;
+            const index = String(iii).padStart(TemplateEngine.#ATTRIBUTE_PADDING, '0');
+            string = string.slice(0, -syntax - attribute.length) + `${prefix}-${index}="${attribute}`;
           } else {
             // We found a match like this: html`<div .title="${value}"></div>`.
             // The syntax takes up 3 characters: `.${property}="`.
             const syntax = 3;
             const prefix = TemplateEngine.#PROPERTY_PREFIX;
-            string = string.slice(0, -syntax - property.length) + `${prefix}-${iii}="${property}`;
+            const index = String(iii).padStart(TemplateEngine.#ATTRIBUTE_PADDING, '0');
+            string = string.slice(0, -syntax - property.length) + `${prefix}-${index}="${property}`;
           }
           state.index = 1; // Accounts for an expected quote character next.
         } else {
@@ -1563,8 +1592,35 @@ class TemplateEngine {
     return template.content;
   }
 
-  static #findLookups(node, nodeType = Node.DOCUMENT_FRAGMENT_NODE, path = []) {
-    const lookups = [];
+  // Walk through our fragment that we added special markers to and collect
+  //  paths to each future target. We use “paths” because each future instance
+  //  will clone this fragment and so paths are all we can really cache. And,
+  //  while we go through, clean up our bespoke markers.
+  // Note that we are always walking the interpolated strings and the resulting,
+  //  instantiated DOM _in the same depth-first manner_. This means that the
+  //  ordering is fairly reliable. The only special handling we need to do is to
+  //  ensure that we don’t rely on the ordering of NamedNodeMap objects since
+  //  the spec doesn’t guarantee anything there (though in practice, it would
+  //  probably work…).
+  //
+  // For example, we walk this structure:
+  //
+  // <div
+  //   id="foo-bar-baz"
+  //   x-element-attribute-000001="foo"
+  //   x-element-attribute-000002="bar"
+  //   x-element-property-000003="baz">
+  //   <!--x-element-content-->
+  // </div>
+  //
+  // And end up with this (which is ready to be injected into a container):
+  //
+  // <div id="foo-bar-baz">
+  //   <!---->
+  //   <!---->
+  // </div>
+  //
+  static #findLookups(node, nodeType = Node.DOCUMENT_FRAGMENT_NODE, lookups = [], path = []) {
     // @ts-ignore — TypeScript doesn’t seem to understand the nodeType param.
     if (nodeType === Node.ELEMENT_NODE) {
       // Copy the live NamedNodeMap since we need to mutate it during iteration.
@@ -1581,8 +1637,9 @@ class TemplateEngine {
                 ? 'defined'
                 : null;
         if (type) {
+          const index = Number(name.slice(-TemplateEngine.#ATTRIBUTE_PADDING));
           const value = attribute.value;
-          lookups.push({ path, type, name: value });
+          lookups[index] = { path, type, name: value };
           node.removeAttribute(name);
         }
       }
@@ -1593,13 +1650,14 @@ class TemplateEngine {
         node.textContent.includes(TemplateEngine.#CONTENT_PREFIX)
       ) {
         throw new Error(`Interpolation of "${localName}" tags is not allowed.`);
-      } else if (
-        localName === 'plaintext' ||
-        localName === 'textarea' ||
-        localName === 'title'
-      ) {
+      } else if (localName === 'textarea' || localName === 'title') {
         if (node.textContent.includes(TemplateEngine.#CONTENT_PREFIX)) {
-          lookups.push({ path, type: 'text' });
+          if (node.textContent === `<!--${TemplateEngine.#CONTENT_PREFIX}-->`) {
+            node.textContent = '';
+            lookups.push({ path, type: 'text' });
+          } else {
+            throw new Error(`Only basic interpolation of "${localName}" tags is allowed.`);
+          }
         }
       }
     } else if (
@@ -1621,13 +1679,18 @@ class TemplateEngine {
       for (const childNode of node.childNodes) {
         const childNodeType = childNode.nodeType;
         if (childNodeType === Node.ELEMENT_NODE || Node.COMMENT_NODE) {
-          lookups.push(...TemplateEngine.#findLookups(childNode, childNodeType, [...path, iii++]));
+          TemplateEngine.#findLookups(childNode, childNodeType, lookups, [...path, iii++]);
         }
       }
     }
-    return lookups;
+    if (nodeType === Node.DOCUMENT_FRAGMENT_NODE) {
+      return lookups;
+    }
   }
 
+  // After cloning our common fragment, we use the “lookups” to cache live
+  //  references to DOM nodes so that we can surgically perform updates later in
+  //  an efficient manner. Lookups are like directions to find our real targets.
   static #findTargets(fragment, lookups) {
     const targets = [];
     const cache = new Map();
@@ -1659,6 +1722,7 @@ class TemplateEngine {
     return targets;
   }
 
+  // Create and prepare a document fragment to be injected into some container.
   static #ready(result) {
     if (result.readied) {
       throw new Error(`Unexpected re-injection of template result.`);
@@ -1678,16 +1742,6 @@ class TemplateEngine {
     Object.assign(result, { fragment, entries });
   }
 
-  static #inject(result, node, options) {
-    const nodes = result.type === 'svg'
-      ? result.fragment.firstChild.childNodes
-      : result.fragment.childNodes;
-    options?.before
-      ? TemplateEngine.#insertAllBefore(node.parentNode, node, nodes)
-      : TemplateEngine.#insertAllBefore(node, null, nodes);
-    result.fragment = null;
-  }
-
   static #assign(result, newResult) {
     result.lastValues = result.values;
     result.values = newResult.values;
@@ -1845,6 +1899,8 @@ class TemplateEngine {
     }
   }
 
+  // Bind the current values from a result by walking through each target and
+  //  updating the DOM if things have changed.
   static #commit(result) {
     result.lastValues ??= result.values.map(() => TemplateEngine.#UNSET);
     const { entries, values, lastValues } = result;
@@ -1862,6 +1918,18 @@ class TemplateEngine {
     }
   }
 
+  // Attach a document fragment into some container. Note that all the DOM in
+  //  the fragment will already have values correctly bound.
+  static #inject(result, node, options) {
+    const nodes = result.type === 'svg'
+      ? result.fragment.firstChild.childNodes
+      : result.fragment.childNodes;
+    options?.before
+      ? TemplateEngine.#insertAllBefore(node.parentNode, node, nodes)
+      : TemplateEngine.#insertAllBefore(node, null, nodes);
+    result.fragment = null;
+  }
+
   static #throwUpdaterError(updater, type) {
     switch (updater) {
       case TemplateEngine.#live:
