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

fix(jqLite): make jqLite invoke jqLite.cleanData as a method #15846

Closed
wants to merge 2 commits into from
Closed

fix(jqLite): make jqLite invoke jqLite.cleanData as a method #15846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
822e745
fix(jqLite): make jqLite invoke jqLite.cleanData as a method
mgol Aug 20, 2014
295cb4d
fixup! fix(jqLite): make jqLite invoke jqLite.cleanData as a method
mgol Mar 22, 2017
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
19 changes: 7 additions & 12 deletions src/jqLite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,12 +201,6 @@ function jqLiteHasData(node) {
return false;
}

function jqLiteCleanData(nodes) {
for (var i = 0, ii = nodes.length; i < ii; i++) {
jqLiteRemoveData(nodes[i]);
}
}

function jqLiteBuildFragment(html, context) {
var tmp, tag, wrap,
fragment = context.createDocumentFragment(),
Expand Down Expand Up @@ -309,13 +303,10 @@ function jqLiteClone(element) {
}

function jqLiteDealoc(element, onlyDescendants) {
if (!onlyDescendants) jqLiteRemoveData(element);
if (!onlyDescendants && jqLiteAcceptsData(element)) jqLite.cleanData([element]);
Copy link
Member Author

Choose a reason for hiding this comment

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

@gkalpak Let me elaborate why it shouldn't break anything now. The only situation where we would fire jqLiteRemoveData(element) before and not fire jqLite.cleanData([element]) here now is when jqLiteAcceptsData(element) is false. But if you then look at jqLiteRemoveData source, it changes anything only if element.ng339 is defined. This property is only set to something non-undefined inside jqLiteExpandoStore and only if the second passed argument is truthy. That, in turn, happens in two places only: one is inside jqLiteData, the other one is in jqLite#on. In both cases this setter is guarded by the jqLiteAcceptsData(element) being truthy which contradicts our assumptions.

Therefore, there should be no observable differences to external code here.


if (element.querySelectorAll) {
var descendants = element.querySelectorAll('*');
for (var i = 0, l = descendants.length; i < l; i++) {
jqLiteRemoveData(descendants[i]);
}
jqLite.cleanData(element.querySelectorAll('*'));
}
}

Expand Down Expand Up @@ -613,7 +604,11 @@ forEach({
data: jqLiteData,
removeData: jqLiteRemoveData,
hasData: jqLiteHasData,
cleanData: jqLiteCleanData
cleanData: function jqLiteCleanData(nodes) {
for (var i = 0, ii = nodes.length; i < ii; i++) {
jqLiteRemoveData(nodes[i]);
}
}
}, function(fn, name) {
JQLite[name] = fn;
});
Expand Down
72 changes: 34 additions & 38 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2100,28 +2100,26 @@ describe('$compile', function() {

it('should work in jqLite and jQuery with jQuery.cleanData last patched by Angular', runTest);

if (jQuery) {
it('should work with another library patching jQuery.cleanData after Angular', function() {
var cleanedCount = 0;
var currentCleanData = jqLite.cleanData;
jqLite.cleanData = function(elems) {
cleanedCount += elems.length;
// Don't return the output and explicitly pass only the first parameter
// so that we're sure we're not relying on either of them. jQuery UI patch
// behaves in this way.
currentCleanData(elems);
};
it('should work with another library patching jqLite/jQuery.cleanData after Angular', function() {
var cleanedCount = 0;
var currentCleanData = jqLite.cleanData;
jqLite.cleanData = function(elems) {
cleanedCount += elems.length;
// Don't return the output and explicitly pass only the first parameter
// so that we're sure we're not relying on either of them. jQuery UI patch
// behaves in this way.
currentCleanData(elems);
};

runTest();
runTest();

// The initial ng-repeat div is dumped after parsing hence we expect cleanData
// count to be one larger than size of the iterated array.
expect(cleanedCount).toBe(is.length + 1);
// The initial ng-repeat div is dumped after parsing hence we expect cleanData
// count to be one larger than size of the iterated array.
expect(cleanedCount).toBe(is.length + 1);

// Restore the previous cleanData.
jqLite.cleanData = currentCleanData;
});
}
// Restore the previous cleanData.
jqLite.cleanData = currentCleanData;
});
});

describe('replace and not exactly one root element', function() {
Expand Down Expand Up @@ -8640,28 +8638,26 @@ describe('$compile', function() {

it('should work without external libraries (except jQuery)', testCleanup);

if (jQuery) {
it('should work with another library patching jQuery.cleanData after AngularJS', function() {
var cleanedCount = 0;
var currentCleanData = jqLite.cleanData;
jqLite.cleanData = function(elems) {
cleanedCount += elems.length;
// Don't return the output and explicitly pass only the first parameter
// so that we're sure we're not relying on either of them. jQuery UI patch
// behaves in this way.
currentCleanData(elems);
};
it('should work with another library patching jqLite/jQuery.cleanData after AngularJS', function() {
var cleanedCount = 0;
var currentCleanData = jqLite.cleanData;
jqLite.cleanData = function(elems) {
cleanedCount += elems.length;
// Don't return the output and explicitly pass only the first parameter
// so that we're sure we're not relying on either of them. jQuery UI patch
// behaves in this way.
currentCleanData(elems);
};

testCleanup();
testCleanup();

// The ng-repeat template is removed/cleaned (the +1)
// and each clone of the ng-repeat template is also removed (xs.length)
expect(cleanedCount).toBe(xs.length + 1);
// The ng-repeat template is removed/cleaned (the +1)
// and each clone of the ng-repeat template is also removed (xs.length)
expect(cleanedCount).toBe(xs.length + 1);

// Restore the previous cleanData.
jqLite.cleanData = currentCleanData;
});
}
// Restore the previous cleanData.
jqLite.cleanData = currentCleanData;
});
});


Expand Down