Upgrade disconnected custom elements before setting properties on them.

[justinfagnani]

This keeps us from creating instance properties that alias assessors on disconnected elements.

This technique only works if customElements.upgrade is implemented. It's in Chrome, the polyfill, and Safari TP, and Firefox nightly.

There's two other ways we could solve this. First we either:

1. Patch TreeWalker properly in ShadyDOM
2. Don't use TreeWalker to traverse, so that we don't traverse ShadyDOM ShadowRoots created synchronously in constructors.

Then we go back to document.importNode(template.content, true) to clone & upgrade in one step.

[justinfagnani]

Tests are failing in Safari 10 as expected. This shows the problem with this approach...

The other solutions are much more invasive though

[ruphin]

I think the proper solution is to defer the responsibility of 'fixing' instance properties to the Custom Element. This is required anyway for Custom Elements to function with deferred upgrades (through e.g. lazy imports). I think the following pattern is fairly reasonable/common:

class MyApp {
  get template() {
    return html`
      <div id="pages">
        <login-page></login-page>
        <main-page user=${user}></main-page>
        <other-page user=${user}></other-page>
     </div>
  `;
  }
  onRouteChange(newPage) {
    import(newPage).then(
      () => {
        this.pages[newPage].setAttribute('active', '');
        this.pages[oldPage].removeAttribute('active');
      }
    );
  }
}

This will break the Custom Elements no matter what lit-html does, because at the time of first render, the page elements are not registered as custom elements (because they are not yet imported).

If the Custom Element baseclass contains something like this in the constructor, it handles all these problems gracefully:

Object.getOwnPropertyNames(this).forEach(property => {
  const propertyValue = this[property];
  delete this[property];
  this[property] = propertyValue;
});

[sorvell]

If we go this route, we need to polyfill on Safari.

[sorvell]

I might be inclined to polyfill TreeWalker in ShadyDOM instead.

[sorvell]

duplicate test name?

[ruphin]

Option two is basically what I implemented in lite-html. In my benchmarks the initialial load of my implementation is consistently faster, and the template processing is the largest component there. I haven't done a comprehensive comparison on code-size, but I think my implementation is both smaller and more performant (although that might be due to some optimizations I can make that are not possible with your architecture).

If you want, I can look into the feasibility of porting my implementation to lit-html, and make a PR for it. It will require quite a bit of work since our AttributePart architecture is quite different, so I won't start with it unless you think it's a reasonable option.

[justinfagnani]

@sorvell PTAL

Changed to call document.importNode in the native custom elements case, where elements cannot spawn children during upgrade, and to force upgrade after instance prep in the polyfill case.

[sorvell]

We should only do this if it's not connected since otherwise it's duplicate work and crawling trees is costly. I think it's fine to merge this and just put a TODO here for now.

[justinfagnani]

Just fixed it here