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.

Sign up for GitHub

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

Jump to bottom

Change findTemplateNode to work on array of indices #4853

Closed
wants to merge 1 commit into from
Closed

Change findTemplateNode to work on array of indices #4853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion externs/polymer-closure-types.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
NodeInfo.prototype.hasInsertionPoint;
/** @type {!TemplateInfo} */
NodeInfo.prototype.templateInfo;
/** @type {!NodeInfo} */
/** @type {!Array<number>} */
NodeInfo.prototype.parentInfo;
/** @type {number} */
NodeInfo.prototype.parentIndex;
Expand Down
29 changes: 13 additions & 16 deletions lib/mixins/template-stamp.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,20 +44,16 @@
}

function findTemplateNode(root, nodeInfo) {
// recursively ascend tree until we hit root
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.parentIndex === i++) {
return n;
let parent = root;
for (let i = 0, l = nodeInfo.length; i < l; i++) {
for (let n = parent.firstChild, j = 0; n; n = n.nextSibling) {
if (nodeInfo[i] === j++) {
parent = n;
break;
}
}
} else {
return root;
}
return parent;
}

// construct `$` map (from id annotations)
Expand Down Expand Up @@ -154,8 +150,7 @@
* templateInfo: {object}, // nested template metadata
* // Metadata to allow efficient retrieval of instanced node
* // corresponding to this metadata
* parentInfo: {number}, // reference to parent nodeInfo>
* parentIndex: {number}, // index in parent's `childNodes` collection
* parentInfo: {Array}, // indices to traverse to this node from the template root
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this is a breaking change. I don't think we can accept this until Polymer 3.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be possible to factor findTemplateNode such that it is still recursive, but able to operate either on a parentInfo tree (for backward compatibility) or a parentIndex array (for efficient serialization). We can consider this if we move forward with #4782 (or a non-breaking refactoring of it). In that way, the runtime code would still generate backward-compatible parentInfo trees (i.e. https://github.com/Polymer/polymer/pull/4853/files#diff-84d426316fb9e8502af8f5eed8675b40L286 would be reverted), and the build-time tool would transform that into an array of indices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could certainly work by being able to parse both recursive notation as well as the array notation, but I don't think it would be good to land that before Polymer 3. Since the build-time tool is likely not ready before 3 is ready, landing it right now increases the Polymer code size and adds more conditions.

* infoIndex: {number}, // index of this `nodeInfo` in `templateInfo.nodeInfoList`
* },
* ...
Expand Down Expand Up @@ -203,7 +198,7 @@
templateInfo.stripWhiteSpace =
(outerTemplateInfo && outerTemplateInfo.stripWhiteSpace) ||
template.hasAttribute('strip-whitespace');
this._parseTemplateContent(template, templateInfo, {parent: null});
this._parseTemplateContent(template, templateInfo, {parentInfo: []});
}
return template._templateInfo;
}
Expand Down Expand Up @@ -283,7 +278,9 @@
continue;
}
}
let childInfo = { parentIndex, parentInfo: nodeInfo };
let childInfo = {
parentInfo: nodeInfo.parentInfo.slice().concat(parentIndex)
};
if (this._parseTemplateNode(node, templateInfo, childInfo)) {
childInfo.infoIndex = templateInfo.nodeInfoList.push(/** @type {!NodeInfo} */(childInfo)) - 1;
}
Expand Down Expand Up @@ -427,7 +424,7 @@
let nodes = dom.nodeList = new Array(nodeInfo.length);
dom.$ = {};
for (let i=0, l=nodeInfo.length, info; (i<l) && (info=nodeInfo[i]); i++) {
let node = nodes[i] = findTemplateNode(dom, info);
let node = nodes[i] = findTemplateNode(dom, info.parentInfo);
applyIdToMap(this, dom.$, node, info);
applyTemplateContent(this, node, info);
applyEventListener(this, node, info);
Expand Down