From f0cbc837d0d512a467b0f2adb5a199bd38a7bec7 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Mon, 1 Jul 2019 17:36:46 -0700 Subject: [PATCH] Updates from review. --- lib/mixins/property-effects.js | 34 ++++++++++++++++++++++++++++------ lib/mixins/template-stamp.js | 7 +++++++ lib/utils/settings.js | 6 +++--- lib/utils/templatize.js | 3 ++- 4 files changed, 40 insertions(+), 10 deletions(-) diff --git a/lib/mixins/property-effects.js b/lib/mixins/property-effects.js index 190690ab7c..3b7d9ee8eb 100644 --- a/lib/mixins/property-effects.js +++ b/lib/mixins/property-effects.js @@ -2691,7 +2691,7 @@ export const PropertyEffects = dedupingMixin(superClass => { } if (instanceBinding) { // For instance-time binding, create instance of template metadata - // and link into list of templates if necessary + // and link into tree of templates if necessary templateInfo = /** @type {!TemplateInfo} */(Object.create(templateInfo)); templateInfo.wasPreBound = wasPreBound; if (!this.__templateInfo) { @@ -2699,9 +2699,16 @@ export const PropertyEffects = dedupingMixin(superClass => { this.__templateInfo = templateInfo; } else { // Append this template info onto the end of its parent template's - // nested template list; if this template was not nested in another + // list, which will determine the tree structure via which property + // effects are run; if this template was not nested in another // template, use the root template (the first stamped one) as the - // parent + // parent. Note, `parent` is the `templateInfo` instance for this + // template's containing template, which was set up in + // `applyTemplateContent`. While a given template's `parent` is + // given, it is only added to the parent list at the point that it is + // being bound, since a template may or may not ever be stamped, and + // may be stamped more than once (in which case it will be in the + // tree more than once). const parent = templateInfo.parent || this.__templateInfo; const previous = parent.lastChild; parent.lastChild = templateInfo; @@ -2800,7 +2807,10 @@ export const PropertyEffects = dedupingMixin(superClass => { * @protected */ _removeBoundDom(dom) { - // Unlink template info + // Unlink template info; Note that while the child is unlinked from its + // parent list, a template's `parent` reference is never removed, since + // this is is determined by the tree structure and applied at + // `applyTemplateContent` time. let templateInfo = dom.templateInfo; const {previousSibling, nextSibling, parent} = templateInfo; if (previousSibling) { @@ -2955,6 +2965,8 @@ export const PropertyEffects = dedupingMixin(superClass => { if (removeNestedTemplates && (isDomIf || isDomRepeat)) { parent.removeChild(node); nodeInfo.parentInfo.templateInfo = nestedTemplateInfo; + // Ensure the parent dom-if/repeat is noted since it now has bindings; + // it may not have been if it did not have its own bindings nodeInfo = nodeInfo.parentInfo; nodeInfo.noted = true; noted = false; @@ -2962,12 +2974,22 @@ export const PropertyEffects = dedupingMixin(superClass => { // Merge host props into outer template and add bindings let hostProps = nestedTemplateInfo.hostProps; if (fastDomIf && isDomIf) { + // `fastDomIf` mode uses runtime-template stamping to add accessors/ + // effects to properties used in its template; as such we don't need to + // tax the host element with `_host_` bindings for the `dom-if`. + // However, in the event it is nested in a `dom-repeat`, it is still + // important that its host properties are added to the + // TemplateInstance's `hostProps` so that they are forwarded to the + // TemplateInstance. if (hostProps) { + templateInfo.hostProps = + Object.assign(templateInfo.hostProps || {}, hostProps); + // Ensure the dom-if is noted so that it has a __dataHost, since + // `fastDomIf` uses the host for runtime template stamping; note this + // was already ensured above in the `removeNestedTemplates` case if (!removeNestedTemplates) { nodeInfo.parentInfo.noted = true; } - templateInfo.hostProps = - Object.assign(templateInfo.hostProps || {}, hostProps); } } else { let mode = '{'; diff --git a/lib/mixins/template-stamp.js b/lib/mixins/template-stamp.js index f2d194f2e7..924b8fa8ea 100644 --- a/lib/mixins/template-stamp.js +++ b/lib/mixins/template-stamp.js @@ -257,6 +257,10 @@ export const TemplateStamp = dedupingMixin( if (element.hasAttributes && element.hasAttributes()) { noted = this._parseTemplateNodeAttributes(element, templateInfo, nodeInfo) || noted; } + // Checking `nodeInfo.noted` allows a child node of this node (who gets + // access to `parentInfo`) to cause the parent to be noted, which + // otherwise has no return path via `_parseTemplateChildNodes` (used by + // some optimizations) return noted || nodeInfo.noted; } @@ -448,6 +452,9 @@ export const TemplateStamp = dedupingMixin( window.HTMLTemplateElement && HTMLTemplateElement.decorate) { HTMLTemplateElement.decorate(template); } + // Accepting the `templateInfo` via an argument allows for creating + // instances of the `templateInfo` by the caller, useful for adding + // instance-time information to the prototypical data templateInfo = templateInfo || this.constructor._parseTemplate(template); let nodeInfo = templateInfo.nodeInfoList; let content = templateInfo.content || template.content; diff --git a/lib/utils/settings.js b/lib/utils/settings.js index 6e61d6cad9..9e9fa3aec1 100644 --- a/lib/utils/settings.js +++ b/lib/utils/settings.js @@ -289,12 +289,12 @@ export let removeNestedTemplates = * Sets `removeNestedTemplates` globally, to eliminate nested templates * inside `dom-if` and `dom-repeat` as part of template parsing. * - * @param {boolean} useCemoveNestedTemplates enable or disable removing nested + * @param {boolean} useRemoveNestedTemplates enable or disable removing nested * templates during parsing * @return {void} */ -export const setRemoveNestedTemplates = function(useCemoveNestedTemplates) { - removeNestedTemplates = useCemoveNestedTemplates; +export const setRemoveNestedTemplates = function(useRemoveNestedTemplates) { + removeNestedTemplates = useRemoveNestedTemplates; }; /** diff --git a/lib/utils/templatize.js b/lib/utils/templatize.js index a51dd76c65..4e42ae6fe1 100644 --- a/lib/utils/templatize.js +++ b/lib/utils/templatize.js @@ -405,7 +405,8 @@ function addPropagateEffects(target, templateInfo, options, methodHost) { // Create a cached subclass of the base custom element class onto which // to put the template-specific propagate effects /** @private */ - klass = class TemplatizedTemplateExtension extends templatizedBase {}; + klass = templateInfo.templatizeTemplateClass = + class TemplatizedTemplateExtension extends templatizedBase {}; } // Add template - >instances effects // and host <- template effects