Skip to content

Commit

Permalink
Address feedback from review:
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@kevinpschaaf
kevinpschaaf committed Apr 13, 2017
1 parent dff5f2b commit b9fafb7
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 56 deletions.
49 changes: 26 additions & 23 deletions lib/mixins/property-effects.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,6 @@
addNotifyListener(node, inst, binding);
}
}
addNotifyListener
node.__dataHost = inst;
}
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand All @@ -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) {
Expand Down
16 changes: 8 additions & 8 deletions lib/mixins/template-stamp.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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>
* },
* ...
* ],
Expand Down Expand Up @@ -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);
Expand All @@ -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++;
}
}

Expand Down
71 changes: 46 additions & 25 deletions test/unit/property-effects-template.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -114,7 +117,6 @@
this.pathChanged = sinon.spy();
this.prop = 'prop';
this.obj = {path: 'obj.path'};
this.shadowTemplates = [];
}
ready() {
super.setAttribute('log', '');
Expand All @@ -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);
});
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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.$);
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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);
});
});
Expand Down

0 comments on commit b9fafb7

Please sign in to comment.