From f920e808892793bd80a8966c541e26f18ac5ffd9 Mon Sep 17 00:00:00 2001 From: Andrew Seier Date: Sat, 16 Nov 2024 10:22:12 -0800 Subject: [PATCH] Adding a couple minor changes for 2.x interface. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 5 ++ test/test-template-engine.js | 101 +++++++++++++++++++++++++++++++ x-element.js | 114 ++++++++++++++++++++++++++++------- 3 files changed, 197 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e81914..f1877ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,9 @@ 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 — ``. Now, you have to fit the + interpolation exactly — ``. ### Deprecated @@ -27,6 +30,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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 index edd1d43..2d89ff4 100644 --- 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` +
+ ${content} +
+ `; + }; + 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``; + }; + 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`please ${content} no`; + }; + 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`
Gotta double-quote those.
`; const container = document.createElement('div'); diff --git a/x-element.js b/x-element.js index 59d976f..bc5ff8a 100644 --- 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: + // + //
+ // ${content} + //
+ // + // … and we would instrument it as follows: + // + //
+ // + //
+ // 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`
`. // 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: + // + //
+ // + //
+ // + // And end up with this (which is ready to be injected into a container): + // + //
+ // + // + //
+ // + 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 === ``) { + 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: