Address feedback from review:

kevinpschaaf · kevinpschaaf · commit b9fafb7e9079 · 2017-04-12T18:17:50.000-07:00
* Refactor `_bindTemplate` to remove problematic `hasCreatedAccessors`
* Remove vestigial `dom` from `_bindTemplate` call
* Rename `_unstampTemplate` to `_removeBoundDom`
* Add `infoIndex` to `nodeInfo` (and renamed parent &amp; 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
diff --git 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
@@ -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: <nested template metadata>,
        *         // Metadata to allow efficient retrieval of instanced node
        *         // corresponding to this metadata
-       *         parent: <reference to parent nodeInfo>,
-       *         index: <integer index in parent's `childNodes` collection>
+       *         parentInfo: <reference to parent nodeInfo>,
+       *         parentIndex: <integer index in parent's `childNodes` collection>
        *       },
        *       ...
        *     ],
@@ -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
@@ -103,6 +103,9 @@
       <x-element id="ifElementLD" prop="{{prop}}" path="{{obj.path}}"></x-element>
     </template>
   </template>
+  <template id="templateWithDifferentProps">
+    <div id="bound">[[otherProp]]</div>
+  </template>
   <script>
   HTMLImports.whenReady(() => {
     class XRuntime extends Polymer.GestureEventListeners(Polymer.Element) {
@@ -114,7 +117,6 @@
         this.pathChanged = sinon.spy();
         this.prop = 'prop';
         this.obj = {path: 'obj.path'};
-        this.shadowTemplates = [];
       }
       ready() {
         super.setAttribute('log', '');
@@ -134,6 +136,11 @@
         this.shadowRoot.appendChild(dom);
         return dom;
       }
+      stampTemplateWithDifferentProps() {
+        let dom = this._stampTemplate(Polymer.DomModule.import(this.localName, '#templateWithDifferentProps'));
+        this.shadowRoot.appendChild(dom);
+        return dom;
+      }
     }
     customElements.define('x-runtime', XRuntime);
   });
@@ -162,7 +169,6 @@
   static _parseTemplateNodeAttribute(node, templateInfo, nodeInfo, name, value) {
     if (name == 'special') {
       nodeInfo.specialAttr = value;
-      nodeInfo.infoIndex = templateInfo.nodeInfoList.length;
       node.removeAttribute('special');
       node.setAttribute('had-special', '');
       return true;
@@ -173,7 +179,6 @@
   static _parseTemplateNode(node, templateInfo, nodeInfo) {
     let noted = super._parseTemplateNode(node, templateInfo, nodeInfo);
     if (node.localName == 'x-special') {
-      nodeInfo.infoIndex = templateInfo.nodeInfoList.length;
       noted = nodeInfo.specialNode = true;
     }
     return noted;
@@ -215,14 +220,14 @@
 <dom-module id="x-binding">
   <template>
     <x-element id="standard1" prop="[[prop]]" path="[[obj.path]]"></x-element>
-    <x-element id="custom1" prop='[{"prop": "prop", "prop2": "prop2"}]'></x-element>
+    <x-element id="custom1" prop='[{"a": "prop", "b": "prop2"}]'></x-element>
     <div>
       <x-element id="standard2" prop="[[prop]]" path="[[obj.path]]"></x-element>
-      <x-element id="custom2" prop='[{"prop": "prop", "prop2": "prop2"}]'></x-element>
+      <x-element id="custom2" prop='[{"a": "prop", "b": "prop2"}]'></x-element>
     </div>
-    <template id="domIf" is="dom-if" if="[[prop]]" restamp>
+    <template id="domIf" is="dom-if" if="[[prop2]]" restamp>
       <x-element id="standard3" prop="[[prop]]" path="[[obj.path]]"></x-element>
-      <x-element id="custom3" prop='[{"prop": "prop", "prop2": "prop2"}]'></x-element>
+      <x-element id="custom3" prop='[{"a": "prop", "b": "prop2"}]'></x-element>
     </template>
   </template>
   <script>
@@ -367,10 +372,13 @@
     assert.equal(stamped[0], el.$.first);
     assert.equal(stamped[1], dom1.$.first);
     // Unstamp
-    el._unstampTemplate(dom1);
+    el._removeBoundDom(dom1);
     for (let n in dom1.$) {
       assert.notOk(dom1.$[n].parentNode, null);
     }
+    stamped = el.shadowRoot.querySelectorAll('x-element#first');
+    assert.equal(stamped.length, 1);
+    assert.equal(stamped[0], el.$.first);
     // Stamp again
     let dom2 = el.stampTemplateFromShadow();
     assertStampingCorrect(el, el.$);
@@ -405,8 +413,8 @@
       'x-runtime|x-element#shadow'
     ]);
     // Unstamp
-    el._unstampTemplate(dom2);
-    el._unstampTemplate(dom3);
+    el._removeBoundDom(dom2);
+    el._removeBoundDom(dom3);
     for (let n in dom2.$) {
       assert.notOk(dom1.$[n].parentNode, null);
     }
@@ -448,7 +456,7 @@
     assert.equal(stamped[0], el.$.first);
     assert.equal(stamped[1], dom1.$.first);
     // Unstamp
-    el._unstampTemplate(dom1);
+    el._removeBoundDom(dom1);
     for (let n in dom1.$) {
       assert.notOk(dom1.$[n].parentNode, null);
     }
@@ -486,8 +494,8 @@
       'x-runtime|x-element#light'
     ]);
     // Unstamp
-    el._unstampTemplate(dom2);
-    el._unstampTemplate(dom3);
+    el._removeBoundDom(dom2);
+    el._removeBoundDom(dom3);
     for (let n in dom2.$) {
       assert.notOk(dom1.$[n].parentNode, null);
     }
@@ -554,6 +562,20 @@
     assertAllPropValues(el, sd, ld, 'path', 'obj.path+++', 4);
   });
 
+  test('accessors for non-prototypically bound properties created', () => {
+    // First element
+    let dom = el.stampTemplateWithDifferentProps();
+    el.otherProp = 'otherProp';
+    assert.equal(dom.$.bound.textContent, 'otherProp');
+    // Second element
+    let el2 = document.createElement('x-runtime');
+    document.body.appendChild(el2);
+    let dom2 = el2.stampTemplateWithDifferentProps();
+    el2.otherProp = 'otherProp';
+    assert.equal(dom2.$.bound.textContent, 'otherProp');
+    document.body.removeChild(el2);
+  });
+
   test('prototypical stamping not affected by runtime stamping', () => {
     assertStampingCorrect(el, el.$);
     let stamped = el.shadowRoot.querySelectorAll('x-element#first');
@@ -665,30 +687,29 @@
     assert.equal(el.$.standard1.path, 'obj.path');
     assert.equal(el.$.standard2.path, 'obj.path');
     assert.equal(el.shadowRoot.querySelector('#standard3').path, 'obj.path');
-    assert.equal(el.$.custom1.prop, 'prop prop2');
-    assert.equal(el.$.custom2.prop, 'prop prop2');
-    assert.equal(el.shadowRoot.querySelector('#custom3').prop, 'prop prop2');
+    assert.equal(el.$.custom1.prop, 'a b');
+    assert.equal(el.$.custom2.prop, 'a b');
+    assert.equal(el.shadowRoot.querySelector('#custom3').prop, 'a b');
     el.prop = false;
-    el.$.domIf.render();
     assert.equal(el.$.standard1.prop, false);
     assert.equal(el.$.standard2.prop, false);
+    assert.equal(el.shadowRoot.querySelector('#standard3').prop, false);
     assert.equal(el.$.standard1.path, 'obj.path');
     assert.equal(el.$.standard2.path, 'obj.path');
-    assert.notOk(el.shadowRoot.querySelector('#standard3'));
-    assert.equal(el.$.custom1.prop, 'prop2');
-    assert.equal(el.$.custom2.prop, 'prop2');
-    assert.equal(el.shadowRoot.querySelector('#custom3'));
+    assert.equal(el.shadowRoot.querySelector('#standard3').path, 'obj.path');
+    assert.equal(el.$.custom1.prop, 'b');
+    assert.equal(el.$.custom2.prop, 'b');
+    assert.equal(el.shadowRoot.querySelector('#custom3').prop, 'b');
     el.prop = true;
-    el.$.domIf.render();
     assert.equal(el.$.standard1.prop, true);
     assert.equal(el.$.standard2.prop, true);
     assert.equal(el.shadowRoot.querySelector('#standard3').prop, true);
     assert.equal(el.$.standard1.path, 'obj.path');
     assert.equal(el.$.standard2.path, 'obj.path');
     assert.equal(el.shadowRoot.querySelector('#standard3').path, 'obj.path');
-    assert.equal(el.$.custom1.prop, 'prop prop2');
-    assert.equal(el.$.custom2.prop, 'prop prop2');
-    assert.equal(el.shadowRoot.querySelector('#custom3').prop, 'prop prop2');
+    assert.equal(el.$.custom1.prop, 'a b');
+    assert.equal(el.$.custom2.prop, 'a b');
+    assert.equal(el.shadowRoot.querySelector('#custom3').prop, 'a b');
     document.body.removeChild(el);
   });
 });
