Rework TemplateInstance cloning

[jridgewell]

Instead of traversing over every single Element, Comment, and Text node, this looks only for comment nodes that mark part positions. This likely allows us to skip tons of nodes in the walker (I'm assuming comments are the least used nodes in a page).

Additionally, I'm using a comment node to mark the spot after a template element, so that we know to go back and traverse the template.

By paying about 120 bytes, we can anywhere between a 2-10x speedup (for every browser) with this: https://jsbench.github.io/#d26f5d42b64a4c21dda947adcaaa1570

Additionally, this allows me to delete a significant amount of code from the shady-render module. We're no longer tied to walking nodes by nodeIndex, so we don't need to update every later part's index when we remove a single node, so removal is pretty trivial. We also don't need to pay for an expensive insertion function anymore.

[ruphin]

Can we use document.createComment(data || '') here so we can drop the '' parameters at most of the createComment calls?

[stramel]

Or (data: string = '') =>

[ruphin]

Yes, a default value would be even better

[jridgewell]

Both increase the size of the main binary (3 and 2 bytes respectively).

[ruphin]

That's surprising, there's like 12 createMarker('') calls so uncompressed the difference is 21 bytes in favour of the default value. I guess ('') compresses really well. :)

[ruphin]

Now that I'm reviewing this code, I'm going through the current TemplateInstance cloning and it's quite weird as it is. I have no idea when we started injecting text and parsing TextNodes, but that is completely unnecessary. In fact, there is a serious bug in the template cloning system right now:

render(html`<p>Hello test=${ "something" }</p>`, document.body);

 > "Hello test$lit$=something"

I'm looking into fixing that right now.

[jridgewell]

In fact, there is a serious bug in the template cloning system right now:

Let's move this discussion to #854.

[ruphin]

I am working on a prototype template parser that uses ideas from this PR to improve the template creation process.

The basic idea is that TemplateFactory (or some other component) uses the TemplateResult StringsArray to determine the type and context of each dynamic value (node, attribute, in-commment, in-style). It constructs an HTML string that contains a minimal set of <!-- marker --> nodes that are previousSiblings to whatever nodes the dynamic parts belong to. It also determines the properties of each dynamic part (attribute name for attribute parts, etc), and outputs an array of Part descriptions that look like this:

{
  type: NodePart,
  index: <n> // n-th marker node
}

{
  type: AttributePart,
  index: <n>, // The attribute binding is on the nextSibling of the n-th marker node
  attributeName: 'attr',
  before: 'string', // The content inside the attribute string before the binding
  after: 'string' // The content inside the attribute string after the binding
}

{
  type: StyleNode,
  index: <n> // The binding is inside the <style> node that is the nextSibling of the n-th marker
  before: 'cssString', // The content inside the <style> node before the binding
  after: 'cssString' // The content inside the <style> node after the binding
}

A Template only needs to hold the TemplateElement constructed with the HTML string, and the above information. With these two values we should be capable to construct TemplateInstances directly and with minimal intervention (only walking comments, no parsing of any content required).

The main complexity is in the HTML parsing that we have to do to figure out the context of these parts, but I believe it is relatively easy to do so. We don't have to implement a 'complete' HTML parser, it only needs to differentiate between different basic contexts (between node tags, inside a node tag between < and >, inside a comment, and inside a style tag). I'm working on a basic implementation for this at the moment, and it seems promising.

I have a hard time judging if this HTML string parsing is going to be more efficient than the current tree walking and parsing, but my intuition tells me that it should be. Especially when considering all the random edge cases we are running into with the current parser.

[ruphin]

Here is a very crude proof of concept: https://codepen.io/ruphin/pen/jJVpNa?editors=0010
It seems to be able to handle almost every weird combination of content that I can throw at it, and correctly parse the context and part type.

It still has one minor issue, it does not insert a marker before nodes with an attribute with "" delimiters, but I should be able to put that in when I clean it up.

It looks like a lot of code, but note that it solves a number of other issues that we currently have workarounds for, for example it removes bound attributes from the template entirely so we don't have problems with IE's attribute rewriting. If we add up all the code for those workarounds and the current implementation, I'm sure we can reach a similar byte size compared to the old implementation.

I have no idea yet about runtime performance. I need to clean this code up and implement a constructor for TemplateInstances and Parts that works with this scheme before I can run any benchmarks.

[jridgewell]

Rebased this, it's ready for a review.