Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
perf(jqLite): optimize element dealocation
Browse files Browse the repository at this point in the history
Iterate only over elements and not nodes since we don't attach data or handlers
to text/comment nodes.
  • Loading branch information
IgorMinar authored and rodyhaddad committed Jun 13, 2014
1 parent a196c8b commit e35abc9
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/jqLite.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,10 @@ function jqLiteClone(element) {

function jqLiteDealoc(element){
jqLiteRemoveData(element);
for ( var i = 0, children = element.childNodes || []; i < children.length; i++) {
jqLiteDealoc(children[i]);
var childElement;
for ( var i = 0, children = element.children, l = (children && children.length) || 0; i < l; i++) {
childElement = children[i];
jqLiteDealoc(childElement);
}
}

Expand Down

4 comments on commit e35abc9

@petebacondarwin
Copy link
Contributor

Choose a reason for hiding this comment

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

@IgorMinar & @rodyhaddad - it is not enough to check adding data, we also create expandoStore data when attaching event handlers to nodes such as comment nodes.
It may be that jQuery doesn't allow adding events to comments either but then we have a problem to clear up element transclusion see #7913

@pentode
Copy link

Choose a reason for hiding this comment

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

Shouldn't this have used "children = element.childNodes" instead of "children = element.children"? When using that, the leak in #7913 doesn't occur.

@petebacondarwin
Copy link
Contributor

Choose a reason for hiding this comment

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

The point of the optimization was not to have to search all nodes, since the belief was that the previous commit no longer allowed data to be attached to non-elements. But this is not the case since we are still attaching event handlers to comment nodes, which also uses the cache.

@pentode
Copy link

Choose a reason for hiding this comment

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

Ah, I see. Sorry for the noise...I'm just getting my head around angular.

Please sign in to comment.