diff --git a/config/minifyHtmlLiteralsRollupPlugin.js b/config/minifyHtmlLiteralsRollupPlugin.js index aa7bfe1..64fe63c 100644 --- a/config/minifyHtmlLiteralsRollupPlugin.js +++ b/config/minifyHtmlLiteralsRollupPlugin.js @@ -4,7 +4,7 @@ export function minifyHtmlLiteralsRollupPlugin () { return { name: 'minify-html-in-tag-template-literals', transform (content, id) { - if (id.includes('PickerTemplate.js')) { + if (content.includes('html`')) { return minifyHTMLLiterals(content, { fileName: id }) diff --git a/src/picker/components/Picker/framework.js b/src/picker/components/Picker/framework.js index 53c0b57..6ee9fd5 100644 --- a/src/picker/components/Picker/framework.js +++ b/src/picker/components/Picker/framework.js @@ -55,7 +55,7 @@ function patchChildren (newChildren, instanceBinding) { needsRerender = doChildrenNeedRerender(targetParentNode, newChildren) } else { // first render of list needsRerender = true - instanceBinding.targetNode = undefined // placeholder comment not needed anymore, free memory + instanceBinding.targetNode = undefined // placeholder node not needed anymore, free memory instanceBinding.targetParentNode = targetParentNode = targetNode.parentNode } // avoid re-rendering list if the dom nodes are exactly the same before and after @@ -102,13 +102,9 @@ function patch (expressions, instanceBindings) { } targetNode.replaceWith(newNode) } else { // primitive - string, number, etc - if (targetNode.nodeType === Node.TEXT_NODE) { // already transformed into a text node - // nodeValue is faster than textContent supposedly https://www.youtube.com/watch?v=LY6y3HbDVmg - targetNode.nodeValue = toString(expression) - } else { // replace comment or whatever was there before with a text node - newNode = document.createTextNode(toString(expression)) - targetNode.replaceWith(newNode) - } + // nodeValue is faster than textContent supposedly https://www.youtube.com/watch?v=LY6y3HbDVmg + // note we may be replacing the value in a placeholder text node + targetNode.nodeValue = toString(expression) } if (newNode) { instanceBinding.targetNode = newNode @@ -203,8 +199,8 @@ function parse (tokens) { bindings.push(binding) if (!withinTag && !withinAttribute) { - // add a placeholder comment that we can find later - htmlString += `` + // Add a placeholder text node, so we can find it later. Note we only support one dynamic child text node + htmlString += ' ' } } @@ -216,21 +212,6 @@ function parse (tokens) { } } -function findPlaceholderComment (element, bindingId) { - // If we had a lot of placeholder comments to find, it would make more sense to build up a map once - // rather than search the DOM every time. But it turns out that we always only have one child, - // and it's the comment node, so searching every time is actually faster. - let childNode = element.firstChild - while (childNode) { - // Note that minify-html-literals has already removed all non-framework comments - // So we just need to look for comments that have exactly the bindingId as its text content - if (childNode.nodeType === Node.COMMENT_NODE && childNode.nodeValue === toString(bindingId)) { - return childNode - } - childNode = childNode.nextSibling - } -} - function traverseAndSetupBindings (dom, elementsToBindings) { const instanceBindings = [] // traverse dom @@ -246,11 +227,23 @@ function traverseAndSetupBindings (dom, elementsToBindings) { const targetNode = binding.attributeName ? element // attribute binding, just use the element itself - : findPlaceholderComment(element, i) // not an attribute binding, so has a placeholder comment + : element.firstChild // not an attribute binding, so has a placeholder text node /* istanbul ignore if */ - if (import.meta.env.MODE !== 'production' && !targetNode) { - throw new Error('targetNode should not be undefined') + if (import.meta.env.MODE !== 'production') { + // We only support exactly one placeholder text node inside an element, which simplifies + // the implementation a lot. Also, minify-html-literals should handle any whitespace + // around the expression, so we should only ever see e.g. `
${expr}
` + if ( + !binding.attributeName && + !(element.childNodes.length === 1 && element.childNodes[0].nodeType === Node.TEXT_NODE) + ) { + throw new Error('framework only supports exactly one dynamic child text node') + } + + if (!targetNode) { + throw new Error('targetNode should not be undefined') + } } const instanceBinding = { diff --git a/test/spec/picker/framework.test.js b/test/spec/picker/framework.test.js index 36a1be9..c644672 100644 --- a/test/spec/picker/framework.test.js +++ b/test/spec/picker/framework.test.js @@ -58,7 +58,26 @@ describe('framework', () => { expect(node.outerHTML).toBe('
foo
') }) - test('render two dynamic expressions inside the same element', () => { + test('dynamic expression with whitespace around it - minifier should be working', () => { + const state = { name: 'foo' } + + const { html } = createFramework(state) + + let node + const render = () => { + node = html`
${state.name}\t\n
` + } + + render() + expect(node.outerHTML).toBe('
foo
') + + state.name = 'baz' + render() + expect(node.outerHTML).toBe('
baz
') + }) + + // Framework no longer supports this since we switched from HTML comments to text nodes + test.skip('render two dynamic expressions inside the same element', () => { const state = { name1: 'foo', name2: 'bar' } const { html } = createFramework(state) @@ -77,7 +96,8 @@ describe('framework', () => { expect(node.outerHTML).toBe('
bazquux
') }) - test('render a mix of dynamic and static text nodes in the same element', () => { + // Framework no longer supports this since we switched from HTML comments to text nodes + test.skip('render a mix of dynamic and static text nodes in the same element', () => { const state = { name1: 'foo', name2: 'bar' } const { html } = createFramework(state)