Skip to content

Commit 2dad618

Browse files
jbedardgkalpak
authored andcommitted
refactor($compile): remove skipDestroyOnNextJQueryCleanData, remove jq data of all replaced nodes
Closes: angular#12094
1 parent 85adf3a commit 2dad618

File tree

5 files changed

+153
-37
lines changed

5 files changed

+153
-37
lines changed

src/.jshintrc

-2
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,6 @@
9494
"VALIDITY_STATE_PROPERTY": false,
9595
"reloadWithDebugInfo": false,
9696

97-
"skipDestroyOnNextJQueryCleanData": true,
98-
9997
"NODE_TYPE_ELEMENT": false,
10098
"NODE_TYPE_ATTRIBUTE": false,
10199
"NODE_TYPE_TEXT": false,

src/Angular.js

+4-9
Original file line numberDiff line numberDiff line change
@@ -1678,7 +1678,6 @@ function snake_case(name, separator) {
16781678
}
16791679

16801680
var bindJQueryFired = false;
1681-
var skipDestroyOnNextJQueryCleanData;
16821681
function bindJQuery() {
16831682
var originalCleanData;
16841683

@@ -1712,15 +1711,11 @@ function bindJQuery() {
17121711
originalCleanData = jQuery.cleanData;
17131712
jQuery.cleanData = function(elems) {
17141713
var events;
1715-
if (!skipDestroyOnNextJQueryCleanData) {
1716-
for (var i = 0, elem; (elem = elems[i]) != null; i++) {
1717-
events = jQuery._data(elem, "events");
1718-
if (events && events.$destroy) {
1719-
jQuery(elem).triggerHandler('$destroy');
1720-
}
1714+
for (var i = 0, elem; (elem = elems[i]) != null; i++) {
1715+
events = jQuery._data(elem, "events");
1716+
if (events && events.$destroy) {
1717+
jQuery(elem).triggerHandler('$destroy');
17211718
}
1722-
} else {
1723-
skipDestroyOnNextJQueryCleanData = false;
17241719
}
17251720
originalCleanData(elems);
17261721
};

src/jqLite.js

+8-1
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,12 @@ function jqLiteHasData(node) {
189189
return false;
190190
}
191191

192+
function jqLiteCleanData(nodes) {
193+
for (var i = 0, ii = nodes.length; i < ii; i++) {
194+
jqLiteRemoveData(nodes[i]);
195+
}
196+
}
197+
192198
function jqLiteBuildFragment(html, context) {
193199
var tmp, tag, wrap,
194200
fragment = context.createDocumentFragment(),
@@ -571,7 +577,8 @@ function getAliasedAttrName(name) {
571577
forEach({
572578
data: jqLiteData,
573579
removeData: jqLiteRemoveData,
574-
hasData: jqLiteHasData
580+
hasData: jqLiteHasData,
581+
cleanData: jqLiteCleanData
575582
}, function(fn, name) {
576583
JQLite[name] = fn;
577584
});

src/ng/compile.js

+16-24
Original file line numberDiff line numberDiff line change
@@ -2634,41 +2634,33 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
26342634
parent.replaceChild(newNode, firstElementToRemove);
26352635
}
26362636

2637-
// TODO(perf): what's this document fragment for? is it needed? can we at least reuse it?
2637+
// Append all the `elementsToRemove` to a fragment. This will...
2638+
// - remove them from the DOM
2639+
// - allow them to still be traversed with .nextSibling
2640+
// - allow a single fragment.qSA to fetch all elements being removed
26382641
var fragment = document.createDocumentFragment();
2639-
fragment.appendChild(firstElementToRemove);
2642+
for (i = 0; i < removeCount; i++) {
2643+
fragment.appendChild(elementsToRemove[i]);
2644+
}
26402645

26412646
if (jqLite.hasData(firstElementToRemove)) {
26422647
// Copy over user data (that includes Angular's $scope etc.). Don't copy private
26432648
// data here because there's no public interface in jQuery to do that and copying over
26442649
// event listeners (which is the main use of private data) wouldn't work anyway.
26452650
jqLite.data(newNode, jqLite.data(firstElementToRemove));
26462651

2647-
// Remove data of the replaced element. We cannot just call .remove()
2648-
// on the element it since that would deallocate scope that is needed
2649-
// for the new node. Instead, remove the data "manually".
2650-
if (!jQuery) {
2651-
delete jqLite.cache[firstElementToRemove[jqLite.expando]];
2652-
} else {
2653-
// jQuery 2.x doesn't expose the data storage. Use jQuery.cleanData to clean up after
2654-
// the replaced element. The cleanData version monkey-patched by Angular would cause
2655-
// the scope to be trashed and we do need the very same scope to work with the new
2656-
// element. However, we cannot just cache the non-patched version and use it here as
2657-
// that would break if another library patches the method after Angular does (one
2658-
// example is jQuery UI). Instead, set a flag indicating scope destroying should be
2659-
// skipped this one time.
2660-
skipDestroyOnNextJQueryCleanData = true;
2661-
jQuery.cleanData([firstElementToRemove]);
2662-
}
2652+
// Remove $destroy event listeners from `firstElementToRemove`
2653+
jqLite(firstElementToRemove).off('$destroy');
26632654
}
26642655

2665-
for (var k = 1, kk = elementsToRemove.length; k < kk; k++) {
2666-
var element = elementsToRemove[k];
2667-
jqLite(element).remove(); // must do this way to clean up expando
2668-
fragment.appendChild(element);
2669-
delete elementsToRemove[k];
2670-
}
2656+
// Cleanup any data/listeners on the elements and children.
2657+
// This includes invoking the $destroy event on any elements with listeners.
2658+
jqLite.cleanData(fragment.querySelectorAll('*'));
26712659

2660+
// Update the jqLite collection to only contain the `newNode`
2661+
for (i = 1; i < removeCount; i++) {
2662+
delete elementsToRemove[i];
2663+
}
26722664
elementsToRemove[0] = newNode;
26732665
elementsToRemove.length = 1;
26742666
}

test/ng/compileSpec.js

+125-1
Original file line numberDiff line numberDiff line change
@@ -5844,7 +5844,9 @@ describe('$compile', function() {
58445844

58455845
testCleanup();
58465846

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

58495851
// Restore the previous jQuery.cleanData.
58505852
jQuery.cleanData = currentCleanData;
@@ -8587,4 +8589,126 @@ describe('$compile', function() {
85878589
expect(element.hasClass('fire')).toBe(true);
85888590
}));
85898591
});
8592+
8593+
describe('element replacement', function() {
8594+
it('should broadcast $destroy only on removed elements, not replaced', function() {
8595+
var linkCalls = [];
8596+
var destroyCalls = [];
8597+
8598+
module(function($compileProvider) {
8599+
$compileProvider.directive('replace', function() {
8600+
return {
8601+
multiElement: true,
8602+
replace: true,
8603+
templateUrl: 'template123'
8604+
};
8605+
});
8606+
8607+
$compileProvider.directive('foo', function() {
8608+
return {
8609+
priority: 1, // before the replace directive
8610+
link: function($scope, $element, $attrs) {
8611+
linkCalls.push($attrs.foo);
8612+
$element.on('$destroy', function() {
8613+
destroyCalls.push($attrs.foo);
8614+
});
8615+
}
8616+
};
8617+
});
8618+
});
8619+
8620+
inject(function($compile, $templateCache, $rootScope) {
8621+
$templateCache.put('template123', '<p></p>');
8622+
8623+
$compile(
8624+
'<div replace-start foo="1"><span foo="1.1"></span></div>' +
8625+
'<div foo="2"><span foo="2.1"></span></div>' +
8626+
'<div replace-end foo="3"><span foo="3.1"></span></div>'
8627+
)($rootScope);
8628+
8629+
expect(linkCalls).toEqual(['2', '3']);
8630+
expect(destroyCalls).toEqual([]);
8631+
$rootScope.$apply();
8632+
expect(linkCalls).toEqual(['2', '3', '1']);
8633+
expect(destroyCalls).toEqual(['2', '3']);
8634+
});
8635+
});
8636+
8637+
function getAll($root) {
8638+
// check for .querySelectorAll to support comment nodes
8639+
return [$root[0]].concat($root[0].querySelectorAll ? sliceArgs($root[0].querySelectorAll('*')) : []);
8640+
}
8641+
8642+
function testCompileLinkDataCleanup(template) {
8643+
inject(function($compile, $rootScope) {
8644+
var toCompile = jqLite(template);
8645+
8646+
var preCompiledChildren = getAll(toCompile);
8647+
forEach(preCompiledChildren, function(element, i) {
8648+
jqLite.data(element, 'foo', 'template#' + i);
8649+
});
8650+
8651+
var linkedElements = $compile(toCompile)($rootScope);
8652+
$rootScope.$apply();
8653+
linkedElements.remove();
8654+
8655+
forEach(preCompiledChildren, function(element, i) {
8656+
expect(jqLite.hasData(element)).toBe(false, "template#" + i);
8657+
});
8658+
forEach(getAll(linkedElements), function(element, i) {
8659+
expect(jqLite.hasData(element)).toBe(false, "linked#" + i);
8660+
});
8661+
});
8662+
}
8663+
it('should clean data of element-transcluded link-cloned elements', function() {
8664+
testCompileLinkDataCleanup('<div><div ng-repeat-start="i in [1,2]"><span></span></div><div ng-repeat-end></div></div>');
8665+
});
8666+
it('should clean data of element-transcluded elements', function() {
8667+
testCompileLinkDataCleanup('<div ng-if-start="false"><span><span/></div><span></span><div ng-if-end><span></span></div>');
8668+
});
8669+
8670+
function testReplaceElementCleanup(dirOptions) {
8671+
var template = '<div></div>';
8672+
module(function($compileProvider) {
8673+
$compileProvider.directive('theDir', function() {
8674+
return {
8675+
multiElement: true,
8676+
replace: dirOptions.replace,
8677+
transclude: dirOptions.transclude,
8678+
template: dirOptions.asyncTemplate ? undefined : template,
8679+
templateUrl: dirOptions.asyncTemplate ? 'the-dir-template-url' : undefined
8680+
};
8681+
});
8682+
});
8683+
inject(function($templateCache, $compile, $rootScope) {
8684+
$templateCache.put('the-dir-template-url', template);
8685+
8686+
testCompileLinkDataCleanup(
8687+
'<div>' +
8688+
'<div the-dir-start><span></span></div>' +
8689+
'<div><span></span><span></span></div>' +
8690+
'<div the-dir-end><span></span></div>' +
8691+
'</div>'
8692+
);
8693+
});
8694+
}
8695+
it('should clean data of elements removed for directive template', function() {
8696+
testReplaceElementCleanup({});
8697+
});
8698+
it('should clean data of elements removed for directive templateUrl', function() {
8699+
testReplaceElementCleanup({asyncTmeplate: true});
8700+
});
8701+
it('should clean data of elements transcluded into directive template', function() {
8702+
testReplaceElementCleanup({transclude: true});
8703+
});
8704+
it('should clean data of elements transcluded into directive templateUrl', function() {
8705+
testReplaceElementCleanup({transclude: true, asyncTmeplate: true});
8706+
});
8707+
it('should clean data of elements replaced with directive template', function() {
8708+
testReplaceElementCleanup({replace: true});
8709+
});
8710+
it('should clean data of elements replaced with directive templateUrl', function() {
8711+
testReplaceElementCleanup({replace: true, asyncTemplate: true});
8712+
});
8713+
});
85908714
});

0 commit comments

Comments
 (0)