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

fix(jQuery): cooperate with other libraries monkey-patching jQuery.cleanData #8486

Merged
merged 1 commit into from
Aug 14, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 2 additions & 0 deletions src/.jshintrc
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@
"getBlockElements": false,
"VALIDITY_STATE_PROPERTY": false,

"skipDestroyOnNextJQueryCleanData": true,

/* filters.js */
"getFirstThursdayOfYear": false,

Expand Down
24 changes: 17 additions & 7 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -1454,8 +1454,15 @@ function snake_case(name, separator) {
});
}

var bindJQueryFired = false;
var skipDestroyOnNextJQueryCleanData;
function bindJQuery() {
var originalCleanData;

if (bindJQueryFired) {
return;
Copy link
Member Author

Choose a reason for hiding this comment

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

@IgorMinar I was wondering why this method was written in a way that is resistant do invoking multiple times. It doesn't seem to be done on the site but only in tests. Such a way of writing this cannot be resistant to other libs monkey-patching cleanData so I had to make sure it's invoked only once.

Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is weird. somehow, sometime in the past we broke this. and I can't even see in git logs when or how it happened.

the way this used to work is that we would try to bind when angular.js is loaded and then again on bootstrap. That way we get to use jQuery even if it was loaded after angular. Obviously things don't work like this any more. It's kind of surprising that nobody has complained about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. There's a larger question of how we want to proceed in various
script configurations. Currently if jQuery is included after Angular,
Angular will just not use it but otherwise it should work.

This PR is aimed at addressing the problem that when anything
monkey-patches jQuery.cleanData after Angular, the code stops working.
jQuery UI is given as an example (this one was actually reported) so one
can think why don't we just disallow it but: 1) it's hard to impossible to
provide a meaningful error message then; 2) it's about other libs as well.

So I think this change is needed but we're fine if we don't use jQuery if
it's loaded after Angular.

If we do want to bring back the support then it's going to be harder to
make it resilient. We could, though, set a flag on the jQuery object saying
if it was patched by us or not. We could then invoke this function as many
times we want and it would patch if and only if the particular jQuery
instance hasn't been patched by us yet.

Your call. :)

On Tue, Aug 12, 2014 at 6:24 PM, Igor Minar [email protected]
wrote:

In src/Angular.js:

function bindJQuery() {

  • var originalCleanData;
  • if (bindJQueryFired) {
  • return;

this is weird. somehow, sometime in the past we broke this. and I can't
even see in git logs when or how it happened.

the way this used to work is that we would try to bind when angular.js is
loaded and then again on bootstrap. That way we get to use jQuery even if
it was loaded after angular. Obviously things don't work like this any
more. It's kind of surprising that nobody has complained about this.


Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/8486/files#r16124157.

Michał Gołębiowski

}

// bind to jQuery if present;
jQuery = window.jQuery;
// Use jQuery if it exists with proper functionality, otherwise default to us.
Expand All @@ -1472,25 +1479,28 @@ function bindJQuery() {
inheritedData: JQLitePrototype.inheritedData
});

originalCleanData = jQuery.cleanData;
// Prevent double-proxying.
originalCleanData = originalCleanData.$$original || originalCleanData;

// All nodes removed from the DOM via various jQuery APIs like .remove()
// are passed through jQuery.cleanData. Monkey-patch this method to fire
// the $destroy event on all removed nodes.
originalCleanData = jQuery.cleanData;
jQuery.cleanData = function(elems) {
for (var i = 0, elem; (elem = elems[i]) != null; i++) {
jQuery(elem).triggerHandler('$destroy');
if (!skipDestroyOnNextJQueryCleanData) {
for (var i = 0, elem; (elem = elems[i]) != null; i++) {
jQuery(elem).triggerHandler('$destroy');
}
} else {
skipDestroyOnNextJQueryCleanData = false;
}
originalCleanData(elems);
};
jQuery.cleanData.$$original = originalCleanData;
} else {
jqLite = JQLite;
}

angular.element = jqLite;

// Prevent double-proxying.
bindJQueryFired = true;
}

/**
Expand Down
14 changes: 9 additions & 5 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -2043,11 +2043,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (!jQuery) {
delete jqLite.cache[firstElementToRemove[jqLite.expando]];
} else {
// jQuery 2.x doesn't expose the data storage. Use jQuery.cleanData to clean up after the replaced
// element. Note that we need to use the original method here and not the one monkey-patched by Angular
// since the patched method emits the $destroy event causing the scope to be trashed and we do need
// the very same scope to work with the new element.
jQuery.cleanData.$$original([firstElementToRemove]);
// jQuery 2.x doesn't expose the data storage. Use jQuery.cleanData to clean up after
// the replaced element. The cleanData version monkey-patched by Angular would cause
// the scope to be trashed and we do need the very same scope to work with the new
// element. However, we cannot just cache the non-patched version and use it here as
// that would break if another library patches the method after Angular does (one
// example is jQuery UI). Instead, set a flag indicating scope destroying should be
// skipped this one time.
skipDestroyOnNextJQueryCleanData = true;
jQuery.cleanData([firstElementToRemove]);
}

for (var k = 1, kk = elementsToRemove.length; k < kk; k++) {
Expand Down
5 changes: 5 additions & 0 deletions test/helpers/testabilityPatch.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@ function dealoc(obj) {

function cleanup(element) {
element.off().removeData();
if (window.jQuery) {
// jQuery 2.x doesn't expose the cache storage; ensure all element data
// is removed during its cleanup.
jQuery.cleanData([element]);
}
// Note: We aren't using element.contents() here. Under jQuery, element.contents() can fail
// for IFRAME elements. jQuery explicitly uses (element.contentDocument ||
// element.contentWindow.document) and both properties are null for IFRAMES that aren't attached
Expand Down
61 changes: 46 additions & 15 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -4118,26 +4118,57 @@ describe('$compile', function() {
});

if (jQuery) {
it('should clean up after a replaced element', inject(function ($compile) {
var privateData, firstRepeatedElem;
describe('cleaning up after a replaced element', function () {
var $compile, xs;
beforeEach(inject(function (_$compile_) {
$compile = _$compile_;
xs = [0, 1];
}));

element = $compile('<div><div ng-repeat="x in xs">{{x}}</div></div>')($rootScope);
function testCleanup() {
var privateData, firstRepeatedElem;

$rootScope.$apply('xs = [0,1]');
firstRepeatedElem = element.children('.ng-scope').eq(0);
element = $compile('<div><div ng-repeat="x in xs">{{x}}</div></div>')($rootScope);

expect(firstRepeatedElem.data('$scope')).toBeDefined();
privateData = jQuery._data(firstRepeatedElem[0]);
expect(privateData.events).toBeDefined();
expect(privateData.events.$destroy).toBeDefined();
expect(privateData.events.$destroy[0]).toBeDefined();
$rootScope.$apply('xs = [' + xs + ']');
firstRepeatedElem = element.children('.ng-scope').eq(0);

$rootScope.$apply('xs = null');
expect(firstRepeatedElem.data('$scope')).toBeDefined();
privateData = jQuery._data(firstRepeatedElem[0]);
expect(privateData.events).toBeDefined();
expect(privateData.events.$destroy).toBeDefined();
expect(privateData.events.$destroy[0]).toBeDefined();

expect(firstRepeatedElem.data('$scope')).not.toBeDefined();
privateData = jQuery._data(firstRepeatedElem[0]);
expect(privateData && privateData.events).not.toBeDefined();
}));
$rootScope.$apply('xs = null');

expect(firstRepeatedElem.data('$scope')).not.toBeDefined();
privateData = jQuery._data(firstRepeatedElem[0]);
expect(privateData && privateData.events).not.toBeDefined();
}

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

it('should work with another library patching jQuery.cleanData after Angular', function () {
var cleanedCount = 0;
var currentCleanData = jQuery.cleanData;
jQuery.cleanData = function (elems) {
cleanedCount += elems.length;
// Don't return the output and expicitly 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();

Copy link
Contributor

Choose a reason for hiding this comment

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

don't you want to make some assertions on cleanedCount?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I totally forgot about that.

// 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(xs.length + 1);

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


Expand Down