Permalink
Browse files

refactor(jqLite): stop patching individual jQuery methods

Currently Angular monkey-patches a few jQuery methods that remove elements
from the DOM. Since methods like .remove() have multiple signatures
that can change what's actually removed, Angular needs to carefully
repeat them in its patching or it can break apps using jQuery correctly.
Such a strategy is also not future-safe.

Instead of patching individual methods on the prototype, it's better to
hook into jQuery.cleanData and trigger custom events there. This should be
safe as e.g. jQuery UI needs it and uses it. It'll also be future-safe.

The only drawback is that $destroy is no longer triggered when using $detach
but:

  1. Angular doesn't use this method, jqLite doesn't implement it.
  2. Detached elements can be re-attached keeping all their events & data
     so it makes sense that $destroy is not triggered on them.
  3. The approach from this commit is so much safer that any issues with
     .detach() working differently are outweighed by the robustness of the code.

BREAKING CHANGE: the $destroy event is no longer triggered when using the
jQuery detach() method. If you want to destroy Angular data attached to the
element, use remove().
  • Loading branch information...
mgol committed Apr 28, 2014
1 parent be7c02c commit d71dbb1ae50f174680533492ce4c7db3ff74df00
Showing with 15 additions and 58 deletions.
  1. +15 −5 src/Angular.js
  2. +0 −43 src/jqLite.js
  3. +0 −10 test/jQueryPatchSpec.js
View
@@ -1430,8 +1430,10 @@ function snake_case(name, separator){
}
function bindJQuery() {
var originalCleanData;
// bind to jQuery if present;
jQuery = window.jQuery;
// reset to jQuery or default to us.
if (jQuery) {
jqLite = jQuery;
@@ -1442,14 +1444,22 @@ function bindJQuery() {
injector: JQLitePrototype.injector,
inheritedData: JQLitePrototype.inheritedData
});
// Method signature:
// jqLitePatchJQueryRemove(name, dispatchThis, filterElems, getterIfNoArguments)
jqLitePatchJQueryRemove('remove', true, true, false);
jqLitePatchJQueryRemove('empty', false, false, false);
jqLitePatchJQueryRemove('html', false, false, true);
originalCleanData = jQuery.cleanData;
// Prevent double-proxying.
originalCleanData = originalCleanData.$$original || originalCleanData;
jQuery.cleanData = function(elems) {
for (var i = 0, elem; (elem = elems[i]) != null; i++) {
jQuery(elem).triggerHandler('$destroy');
}
originalCleanData(elems);
};
jQuery.cleanData.$$original = originalCleanData;
} else {
jqLite = JQLite;
}
angular.element = jqLite;
}
View
@@ -136,49 +136,6 @@ function camelCase(name) {
replace(MOZ_HACK_REGEXP, 'Moz$1');
}
/////////////////////////////////////////////
// jQuery mutation patch
//
// In conjunction with bindJQuery intercepts all jQuery's DOM destruction apis and fires a
// $destroy event on all DOM nodes being removed.
//
/////////////////////////////////////////////
function jqLitePatchJQueryRemove(name, dispatchThis, filterElems, getterIfNoArguments) {
var originalJqFn = jQuery.fn[name];
originalJqFn = originalJqFn.$original || originalJqFn;
removePatch.$original = originalJqFn;
jQuery.fn[name] = removePatch;
function removePatch(param) {
// jshint -W040
var list = filterElems && param ? [this.filter(param)] : [this],
fireEvent = dispatchThis,
set, setIndex, setLength,
element, childIndex, childLength, children;
if (!getterIfNoArguments || param != null) {
while(list.length) {
set = list.shift();
for(setIndex = 0, setLength = set.length; setIndex < setLength; setIndex++) {
element = jqLite(set[setIndex]);
if (fireEvent) {
element.triggerHandler('$destroy');
} else {
fireEvent = !fireEvent;
}
for(childIndex = 0, childLength = (children = element.children()).length;
childIndex < childLength;
childIndex++) {
list.push(jQuery(children[childIndex]));
}
}
}
}
return originalJqFn.apply(this, arguments);
}
}
var SINGLE_TAG_REGEXP = /^<(\w+)\s*\/?>(?:<\/\1>|)$/;
var HTML_REGEXP = /<|&#?\w+;/;
var TAG_NAME_REGEXP = /<([\w:]+)/;
View
@@ -30,10 +30,6 @@ if (window.jQuery) {
describe('$detach event', function() {
it('should fire on detach()', function() {
doc.find('span').detach();
});
it('should fire on remove()', function() {
doc.find('span').remove();
});
@@ -83,12 +79,6 @@ if (window.jQuery) {
describe('$detach event is not invoked in too many cases', function() {
it('should fire only on matched elements on detach(selector)', function() {
doc.find('span').detach('.second');
expect(spy2).toHaveBeenCalled();
expect(spy2.callCount).toEqual(1);
});
it('should fire only on matched elements on remove(selector)', function() {
doc.find('span').remove('.second');
expect(spy2).toHaveBeenCalled();

15 comments on commit d71dbb1

@dmaj7no5th

This comment has been minimized.

Show comment
Hide comment
@dmaj7no5th

dmaj7no5th Jun 9, 2014

Can this be ported to the 1.2x branch? The current monkeypatching in that branch is giving me problems with jquery 1.8.x on IE9

Can this be ported to the 1.2x branch? The current monkeypatching in that branch is giving me problems with jquery 1.8.x on IE9

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 9, 2014

Member

@dmaj7no5th What problems, can you report them? @IgorMinar would have to decide but we generally don't backport refactorings that aren't bug fixes. If it indeed fixes a bug you experience, this may be an incentive.

You can try to cherry-pick this commit onto the v1.2.x branch, create a package via npm i && grunt and see if it helps. Such info would be useful.

Member

mgol replied Jun 9, 2014

@dmaj7no5th What problems, can you report them? @IgorMinar would have to decide but we generally don't backport refactorings that aren't bug fixes. If it indeed fixes a bug you experience, this may be an incentive.

You can try to cherry-pick this commit onto the v1.2.x branch, create a package via npm i && grunt and see if it helps. Such info would be useful.

@dmaj7no5th

This comment has been minimized.

Show comment
Hide comment
@dmaj7no5th

dmaj7no5th Jun 9, 2014

Specifically, using ngRoute. On route changes, when ngRoute cleans up the old view, it calls element.remove(). Somewhere within the mutation patch above (removed in this commit), IE9 throws a "Access is Denied" error. Using angular 1.2.16 with jQuery 1.8.3. Commenting out the monkeypatch makes the routeChange work without error.

I'll see if I can reproduce in a clean app and report an issue

Specifically, using ngRoute. On route changes, when ngRoute cleans up the old view, it calls element.remove(). Somewhere within the mutation patch above (removed in this commit), IE9 throws a "Access is Denied" error. Using angular 1.2.16 with jQuery 1.8.3. Commenting out the monkeypatch makes the routeChange work without error.

I'll see if I can reproduce in a clean app and report an issue

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Jun 10, 2014

Member

can you upgrade to jquery 1.10.x? We don't test against 1.8.3.

Member

IgorMinar replied Jun 10, 2014

can you upgrade to jquery 1.10.x? We don't test against 1.8.3.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 10, 2014

Member

@IgorMinar

can you upgrade to jquery 1.10.x? We don't test against 1.8.3.

That's true, though we officialy state jQuery 1.7 or higher is required and I don't think we can just say we support only the latest version since not all projects can update in this pace.

Also, why 1.10.x and not 1.11.1?

Member

mgol replied Jun 10, 2014

@IgorMinar

can you upgrade to jquery 1.10.x? We don't test against 1.8.3.

That's true, though we officialy state jQuery 1.7 or higher is required and I don't think we can just say we support only the latest version since not all projects can update in this pace.

Also, why 1.10.x and not 1.11.1?

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Jun 10, 2014

Member
Member

IgorMinar replied Jun 10, 2014

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 10, 2014

Member

@dmaj7no5th Doesn't IE9 print any specific line where the problem arises? That would make it easier to figure out. Also, just tu make sure: are you running your app via a local file server and not just via opening static HTML files?

Member

mgol replied Jun 10, 2014

@dmaj7no5th Doesn't IE9 print any specific line where the problem arises? That would make it easier to figure out. Also, just tu make sure: are you running your app via a local file server and not just via opening static HTML files?

@dmaj7no5th

This comment has been minimized.

Show comment
Hide comment
@dmaj7no5th

dmaj7no5th Jun 10, 2014

Upgrading jQuery doesn't fix the problem. After some more debugging it seems the problem goes away if I disable my CSS. I'm not using the ngAnimate module, nor do I have any CSS transitions enabled - but it appears something's going on with the CSS that's breaking ngRoute somehow, and due to that, IE9 won't let jQuery remove the DOM elements.

Upgrading jQuery doesn't fix the problem. After some more debugging it seems the problem goes away if I disable my CSS. I'm not using the ngAnimate module, nor do I have any CSS transitions enabled - but it appears something's going on with the CSS that's breaking ngRoute somehow, and due to that, IE9 won't let jQuery remove the DOM elements.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 10, 2014

Member
Member

mgol replied Jun 10, 2014

@dmaj7no5th

This comment has been minimized.

Show comment
Hide comment
@dmaj7no5th

dmaj7no5th Jun 10, 2014

The debugger doesn't give me any line numbers. I've been trying different settings in Internet Options, but all I can get out of it is "Permission Denied." Once the error occurrs, I have 2 ng-view divs in the DOM.
$('[ng-view]').remove() will throw the same "Permission Denied" error again and again once it occurs.
I'm grasping at straws with the debugger to find exactly what line it's happening on without much luck

The debugger doesn't give me any line numbers. I've been trying different settings in Internet Options, but all I can get out of it is "Permission Denied." Once the error occurrs, I have 2 ng-view divs in the DOM.
$('[ng-view]').remove() will throw the same "Permission Denied" error again and again once it occurs.
I'm grasping at straws with the debugger to find exactly what line it's happening on without much luck

@rwlogel

This comment has been minimized.

Show comment
Hide comment
@rwlogel

rwlogel Jul 31, 2014

Was any decision made about porting this into the 1.2.x branch? I have a directive that uses detach to relocate an element in the DOM and it worked in version 1.2.12, before the $destroy method was corrected. If this change could be ported into 1.2.x and the detach() didn't trigger the $destroy at all it would work again.

Was any decision made about porting this into the 1.2.x branch? I have a directive that uses detach to relocate an element in the DOM and it worked in version 1.2.12, before the $destroy method was corrected. If this change could be ported into 1.2.x and the detach() didn't trigger the $destroy at all it would work again.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jul 31, 2014

Member

@IgorMinar I think we should backport this fix to 1.2.x. This is technically a breaking change but the current behavior is just a bug as it doesn't align with the jQuery meaning of detach. There seem to be enough people affected by past tightening up leaks that hit .detach() that it IMO makes sense to fix them with this patch.

An alternative approach would be to just modify the jQuery patch in Angular 1.2 to not trigger $destroy when using .detach() without switching to the cleanData cleaner way from Angular 1.3.

Anyway, IMO we should do one of these things. I prefer the former but it's your call.

Member

mgol replied Jul 31, 2014

@IgorMinar I think we should backport this fix to 1.2.x. This is technically a breaking change but the current behavior is just a bug as it doesn't align with the jQuery meaning of detach. There seem to be enough people affected by past tightening up leaks that hit .detach() that it IMO makes sense to fix them with this patch.

An alternative approach would be to just modify the jQuery patch in Angular 1.2 to not trigger $destroy when using .detach() without switching to the cleanData cleaner way from Angular 1.3.

Anyway, IMO we should do one of these things. I prefer the former but it's your call.

@rwlogel

This comment has been minimized.

Show comment
Hide comment
@rwlogel

rwlogel Jul 31, 2014

I agree that a simpler patch might be appropriate, in the jqLitePatchJQueryRemove function if the removePatch(param) method was changed to removePatch(param, keepData) and keepData was used to either exit the function immediately or just prevent the the $destroy from triggering that should fix it.

I agree that a simpler patch might be appropriate, in the jqLitePatchJQueryRemove function if the removePatch(param) method was changed to removePatch(param, keepData) and keepData was used to either exit the function immediately or just prevent the the $destroy from triggering that should fix it.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jul 31, 2014

Member

@rwlogel Care to submit a PR? :) Link to it from here.

Maybe it's indeed a better way to not change the whole patch in a patch release if a simpler fix exists.

Member

mgol replied Jul 31, 2014

@rwlogel Care to submit a PR? :) Link to it from here.

Maybe it's indeed a better way to not change the whole patch in a patch release if a simpler fix exists.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Aug 13, 2014

Member

@rwlogel Do you want to tackle it?

Member

mgol replied Aug 13, 2014

@rwlogel Do you want to tackle it?

Please sign in to comment.