Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt+upgrade template fragments after processing for parts #831

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

jridgewell
Copy link
Contributor

Importing the node can cause any custom elements to upgrade, and non-compliant custom elements can cause DOM mutations.

If this happens before we process the template parts, our part.indexs walking will be broken. So, it has to wait till afterwards.

But we still have to return upgraded elements before the template part's interact with the DOM. Luckily, we just adopt and upgrade them after the processing.

Fixes #561.

Importing the node can cause any custom elements to upgrade, and non-compliant custom elements can cause DOM mutations.

If this happens before we process the template parts, our `part.index`s walking will be broken. So, it has to wait till afterwards.

But we still have to return upgraded elements before the template part's interact with the DOM. Luckily, we just adopt and upgrade them after the processing.

Fixes lit#561.
@ruphin
Copy link
Contributor

ruphin commented Feb 25, 2019

The original issue outlined in #561 is that we don't know if the clone-then-adopt approach has a significant performance impact compared to the direct import we have now.

This change does simplify Custom Element handling and fixes several edge cases, but we should get some benchmarks and measure the performance impact before making this change.

@jridgewell
Copy link
Contributor Author

https://jsbench.github.io/#7949baf66c5f9a0a49f478c2a3ee927e

I can't really determine if it's faster or slower, the stddev is really high. Might be due to GC?

@justinfagnani
Copy link
Collaborator

This is the more correct ordering, so ultimately I'd like to take this change.

On the micro benchmark, it seems clear that in the presence of custom elements, the difference is negligible, and neither benchmark have real script work in between steps, which might reduce the difference even further into noise.

@justinfagnani justinfagnani merged commit 5840b3a into lit:master Feb 27, 2019
@jridgewell jridgewell deleted the defer-template-upgrade branch February 27, 2019 21:16
@stramel stramel mentioned this pull request Mar 3, 2019
neuronetio pushed a commit to neuronetio/lit-html that referenced this pull request Dec 2, 2019
Importing the node can cause any custom elements to upgrade, and non-compliant custom elements can cause DOM mutations.

If this happens before we process the template parts, our `part.index`s walking will be broken. So, it has to wait till afterwards.

But we still have to return upgraded elements before the template part's interact with the DOM. Luckily, we just adopt and upgrade them after the processing.

Fixes lit#561.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants