fix: avoid HTML comments, simplify replacement logic (#409)
This commit is contained in:
parent
ff88212004
commit
ce950ff740
|
@ -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
|
||||
})
|
||||
|
|
|
@ -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 += `<!--${bindings.length - 1}-->`
|
||||
// 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. `<div>${expr}</div>`
|
||||
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 = {
|
||||
|
|
|
@ -58,7 +58,26 @@ describe('framework', () => {
|
|||
expect(node.outerHTML).toBe('<div><span>foo</span></div>')
|
||||
})
|
||||
|
||||
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`<div> ${state.name}\t\n</div>`
|
||||
}
|
||||
|
||||
render()
|
||||
expect(node.outerHTML).toBe('<div>foo</div>')
|
||||
|
||||
state.name = 'baz'
|
||||
render()
|
||||
expect(node.outerHTML).toBe('<div>baz</div>')
|
||||
})
|
||||
|
||||
// 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('<div>bazquux</div>')
|
||||
})
|
||||
|
||||
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)
|
||||
|
|
Loading…
Reference in New Issue