From b9fafb7e9079b1f0aa1211238c524303e4e54296 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 12 Apr 2017 18:17:50 -0700 Subject: [PATCH] Address feedback from review: * Refactor `_bindTemplate` to remove problematic `hasCreatedAccessors` * Remove vestigial `dom` from `_bindTemplate` call * Rename `_unstampTemplate` to `_removeBoundDom` * Add `infoIndex` to `nodeInfo` (and renamed parent & index) * Add test to ensure runtime accessors created for new props in runtime stamped template * Changed custom binding test to use different prop names * Added test for #first count after removing bound dom --- lib/mixins/property-effects.html | 49 ++++++++-------- lib/mixins/template-stamp.html | 16 +++--- test/unit/property-effects-template.html | 71 +++++++++++++++--------- 3 files changed, 80 insertions(+), 56 deletions(-) diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 435e558d4a..06ef7eeded 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -645,7 +645,6 @@ addNotifyListener(node, inst, binding); } } - addNotifyListener node.__dataHost = inst; } } @@ -1999,35 +1998,40 @@ * templates can be efficiently stamped and unstamped. * * This method may be called on the prototype (for prototypical template - * binding) once per prototype, or on the instance for either the - * prototypically bound template and/or one or more templates to stamp - * bound to the instance. - * - * Note that a template may only be stamped by one element; stamping - * the same template by multiple element types is not currently supported. + * binding, to avoid creating accessors every instance) once per prototype, + * and will be called with `runtimeBinding: true` by `_stampTemplate` to + * create and link an instance of the template metadata associated with a + * particular stamping. * * @param {HTMLTemplateElement} template Template containing binding * bindings - * @param {DocumentFragment=} dom Stamped DOM for this template to be - * bound to the instance (leave undefined for prototypical template - * binding) - * @return {Object} Instanced template metadata object + * @param {boolean=} runtimeBinding When false (default), performs + * "prototypical" binding of the template and overwrites any previously + * bound template for the class. When true (as passed from + * `_stampTemplate`), the template info is instanced and linked into + * the list of bound templates. + * @return {Object} Template metadata object; for `runtimeBinding`, + * this is an instance of the prototypical template info * @protected */ - _bindTemplate(template) { + _bindTemplate(template, instanceBinding) { let templateInfo = this.constructor._parseTemplate(template); - if (!templateInfo.hasCreatedAccessors) { - // Create accessors for any template effects + let wasPreBound = this.__templateInfo == templateInfo; + // Optimization: since this is called twice for proto-bound templates, + // don't attempt to recreate accessors if this template was pre-bound + if (!wasPreBound) { for (let prop in templateInfo.propertyEffects) { this._createPropertyAccessor(prop); } - templateInfo.hasCreatedAccessors = true; } - // Create instance of template metadata and link into list of templates - templateInfo = Object.create(templateInfo); - if (this.hasOwnProperty('__templateInfo')) { - templateInfo.nextTemplateInfo = this.__templateInfo; - this.__templateInfo.previousTemplateInfo = templateInfo; + if (instanceBinding) { + // For instance-time binding, create instance of template metadata + // and link into list of templates if necessary + templateInfo = Object.create(templateInfo); + if (!wasPreBound && this.__templateInfo) { + templateInfo.nextTemplateInfo = this.__templateInfo; + this.__templateInfo.previousTemplateInfo = templateInfo; + } } return this.__templateInfo = templateInfo; } @@ -2072,8 +2076,7 @@ */ _stampTemplate(template) { let dom = super._stampTemplate(template); - // Link template info & create accessors - let templateInfo = this._bindTemplate(template, dom); + let templateInfo = this._bindTemplate(template, true); // Add template-instance-specific data to instanced templateInfo templateInfo.nodeList = dom.nodeList; templateInfo.childNodes = Array.from(dom.childNodes); @@ -2096,7 +2099,7 @@ * from `_stampTemplate` associated with the nodes to be removed * @protected */ - _unstampTemplate(dom) { + _removeBoundDom(dom) { // Unlink template info let templateInfo = dom.templateInfo; if (templateInfo.previousTemplateInfo) { diff --git a/lib/mixins/template-stamp.html b/lib/mixins/template-stamp.html index 7123f156c4..50cf1783dc 100644 --- a/lib/mixins/template-stamp.html +++ b/lib/mixins/template-stamp.html @@ -45,13 +45,13 @@ function findTemplateNode(root, nodeInfo) { // recursively ascend tree until we hit root - let parent = nodeInfo.parent && findTemplateNode(root, nodeInfo.parent); + let parent = nodeInfo.parentInfo && findTemplateNode(root, nodeInfo.parentInfo); // unwind the stack, returning the indexed node at each level if (parent) { // note: marginally faster than indexing via childNodes // (http://jsperf.com/childnodes-lookup) for (let n=parent.firstChild, i=0; n; n=n.nextSibling) { - if (nodeInfo.index === i++) { + if (nodeInfo.parentIndex === i++) { return n; } } @@ -152,8 +152,8 @@ * templateInfo: , * // Metadata to allow efficient retrieval of instanced node * // corresponding to this metadata - * parent: , - * index: + * parentInfo: , + * parentIndex: * }, * ... * ], @@ -248,7 +248,7 @@ * @param {Object} nodeInfo Node metadata for current template. */ static _parseTemplateChildNodes(root, templateInfo, nodeInfo) { - for (let node=root.firstChild, index=0, next; node; node=next) { + for (let node=root.firstChild, parentIndex=0, next; node; node=next) { // Wrap templates if (node.localName == 'template') { node = wrapTemplateExtension(node); @@ -272,11 +272,11 @@ continue; } } - let childInfo = { index, parent: nodeInfo }; + let childInfo = { parentIndex, parentInfo: nodeInfo }; if (this._parseTemplateNode(node, templateInfo, childInfo)) { - templateInfo.nodeInfoList.push(childInfo); + childInfo.infoIndex = templateInfo.nodeInfoList.push(childInfo) - 1; } - index++; + parentIndex++; } } diff --git a/test/unit/property-effects-template.html b/test/unit/property-effects-template.html index 5e0a855793..216958367c 100644 --- a/test/unit/property-effects-template.html +++ b/test/unit/property-effects-template.html @@ -103,6 +103,9 @@ +