refactor($compile): remove skipDestroyOnNextJQueryCleanData, remove jq data of all replaced nodes

Author: jbedard

src/.jshintrc

@@ -94,8 +94,6 @@
     "VALIDITY_STATE_PROPERTY": false,
     "reloadWithDebugInfo": false,
 
-    "skipDestroyOnNextJQueryCleanData": true,
-
     "NODE_TYPE_ELEMENT": false,
     "NODE_TYPE_ATTRIBUTE": false,
     "NODE_TYPE_TEXT": false,

src/Angular.js

@@ -1653,7 +1653,6 @@ function snake_case(name, separator) {
 }
 
 var bindJQueryFired = false;
-var skipDestroyOnNextJQueryCleanData;
 function bindJQuery() {
   var originalCleanData;
 
@@ -1688,15 +1687,11 @@ function bindJQuery() {
     originalCleanData = jQuery.cleanData;
     jQuery.cleanData = function(elems) {
       var events;
-      if (!skipDestroyOnNextJQueryCleanData) {
-        for (var i = 0, elem; (elem = elems[i]) != null; i++) {
-          events = jQuery._data(elem, "events");
-          if (events && events.$destroy) {
-            jQuery(elem).triggerHandler('$destroy');
-          }
+      for (var i = 0, elem; (elem = elems[i]) != null; i++) {
+        events = jQuery._data(elem, "events");
+        if (events && events.$destroy) {
+          jQuery(elem).triggerHandler('$destroy');
         }
-      } else {
-        skipDestroyOnNextJQueryCleanData = false;
       }
       originalCleanData(elems);
     };

src/jqLite.js

@@ -189,6 +189,12 @@ 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(),
@@ -564,7 +570,8 @@ function getAliasedAttrName(element, name) {
 forEach({
   data: jqLiteData,
   removeData: jqLiteRemoveData,
-  hasData: jqLiteHasData
+  hasData: jqLiteHasData,
+  cleanData: jqLiteCleanData
 }, function(fn, name) {
   JQLite[name] = fn;
 });

src/ng/compile.js

@@ -2501,41 +2501,34 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
         parent.replaceChild(newNode, firstElementToRemove);
       }
 
-      // TODO(perf): what's this document fragment for? is it needed? can we at least reuse it?
+      // Append all the `elementsToRemove` to a fragment. This will...
+      // - remove them from the DOM
+      // - allow them to still be traversed with .nextSibling
+      // - allow a single fragment.qSA to fetch all elements being removed
       var fragment = document.createDocumentFragment();
-      fragment.appendChild(firstElementToRemove);
+      for (i = 0; i < removeCount; i++) {
+        fragment.appendChild(elementsToRemove[i]);
+      }
 
       if (jqLite.hasData(firstElementToRemove)) {
         // Copy over user data (that includes Angular's $scope etc.). Don't copy private
         // data here because there's no public interface in jQuery to do that and copying over
         // event listeners (which is the main use of private data) wouldn't work anyway.
         jqLite(newNode).data(jqLite(firstElementToRemove).data());
 
-        // Remove data of the replaced element. We cannot just call .remove()
-        // on the element it since that would deallocate scope that is needed
-        // for the new node. Instead, remove the data "manually".
-        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. 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]);
-        }
+        // Remove $destroy event listeners from `firstElementToRemove` since it is being
+        // replaced, not destroyed.
+        jqLite(firstElementToRemove).off('$destroy');
       }
 
-      for (var k = 1, kk = elementsToRemove.length; k < kk; k++) {
-        var element = elementsToRemove[k];
-        jqLite(element).remove(); // must do this way to clean up expando
-        fragment.appendChild(element);
-        delete elementsToRemove[k];
-      }
+      // Cleanup any data/listeners on the elements and children.
+      // This includes invoking the $destroy event on any elements with listeners.
+      jqLite.cleanData(fragment.querySelectorAll('*'));
 
+      // Update the jqLite collection to only contain the `newNode`
+      for (i = 1; i < removeCount; i++) {
+        delete elementsToRemove[i];
+      }
       elementsToRemove[0] = newNode;
       elementsToRemove.length = 1;
     }

test/ng/compileSpec.js

@@ -5676,7 +5676,9 @@ describe('$compile', function() {
 
             testCleanup();
 
-            expect(cleanedCount).toBe(xs.length);
+            // 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 jQuery.cleanData.
             jQuery.cleanData = currentCleanData;
@@ -7889,7 +7891,7 @@ describe('$compile', function() {
 
         $compileProvider.directive('foo', function() {
           return {
-            priority: 1, //before the replace directive
+            priority: 1, // before the replace directive
             link: function($scope, $element, $attrs) {
               linkCalls.push($attrs.foo);
               $element.on('$destroy', function() {
@@ -7901,7 +7903,7 @@ describe('$compile', function() {
       });
 
       inject(function($compile, $templateCache, $rootScope) {
-        $templateCache.put('template123', '<div></div>');
+        $templateCache.put('template123', '<p></p>');
 
         $compile(
           '<div replace-start foo="1"><span foo="1.1"></span></div>' +
@@ -7918,6 +7920,7 @@ describe('$compile', function() {
     });
 
     function getAll($root) {
+      // check for .querySelectorAll to support comment nodes
       return [$root[0]].concat($root[0].querySelectorAll ? sliceArgs($root[0].querySelectorAll('*')) : []);
     }
 
@@ -7949,16 +7952,16 @@ describe('$compile', function() {
       testCompileLinkDataCleanup('<div ng-if-start="false"><span><span/></div><span></span><div ng-if-end><span></span></div>');
     });
 
-    function testReplaceElementCleanup(replace, transclude, asyncTemplate) {
+    function testReplaceElementCleanup(dirOptions) {
       var template = '<div></div>';
       module(function($compileProvider) {
         $compileProvider.directive('theDir', function() {
           return {
             multiElement: true,
-            replace: replace,
-            transclude: transclude,
-            template: asyncTemplate ? undefined : template,
-            templateUrl: asyncTemplate ? 'the-dir-template-url' : undefined
+            replace: dirOptions.replace,
+            transclude: dirOptions.transclude,
+            template: dirOptions.asyncTemplate ? undefined : template,
+            templateUrl: dirOptions.asyncTemplate ? 'the-dir-template-url' : undefined
           };
         });
       });
@@ -7975,22 +7978,22 @@ describe('$compile', function() {
       });
     }
     it('should clean data of elements removed for directive template', function() {
-      testReplaceElementCleanup(false, false, false);
+      testReplaceElementCleanup({});
     });
     it('should clean data of elements removed for directive templateUrl', function() {
-      testReplaceElementCleanup(false, false, true);
+      testReplaceElementCleanup({asyncTmeplate: true});
     });
     it('should clean data of elements transcluded into directive template', function() {
-      testReplaceElementCleanup(false, true, false);
+      testReplaceElementCleanup({transclude: true});
     });
     it('should clean data of elements transcluded into directive templateUrl', function() {
-      testReplaceElementCleanup(false, true, true);
+      testReplaceElementCleanup({transclude: true, asyncTmeplate: true});
     });
     it('should clean data of elements replaced with directive template', function() {
-      testReplaceElementCleanup(true, false, false);
+      testReplaceElementCleanup({replace: true});
     });
     it('should clean data of elements replaced with directive templateUrl', function() {
-      testReplaceElementCleanup(true, false, true);
+      testReplaceElementCleanup({replace: true, asyncTemplate: true});
     });
   });
 });