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().

Author: mgol

src/Angular.js

@@ -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;
 }
 

src/jqLite.js

@@ -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:]+)/;

test/jQueryPatchSpec.js

@@ -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();