From 393fe444d1c89caa7f2d526369a62e6aa3b4e02c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Go=C5=82e=CC=A8biowski?= Date: Wed, 16 Oct 2013 15:15:21 +0200 Subject: [PATCH] =?UTF-8?q?perf(jqLite):=20implement=20and=20use=20the=20`?= =?UTF-8?q?empty`=20method=20in=20place=20of=20`html(=E2=80=98=E2=80=99)`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit jQuery's elem.html('') is way slower than elem.empty(). As clearing element contents happens quite often in certain scenarios, switching to using .empty() provides a significant performance boost when using Angular with jQuery. --- docs/component-spec/annotationsSpec.js | 2 +- .../angular-bootstrap/bootstrap-prettify.js | 2 +- .../guide/dev_guide.unit-testing.ngdoc | 2 +- src/Angular.js | 2 +- src/jqLite.js | 22 +++++++++++++--- src/ng/compile.js | 4 +-- src/ng/directive/ngTransclude.js | 2 +- src/ng/directive/select.js | 4 +-- src/ngAnimate/animate.js | 2 +- test/helpers/testabilityPatch.js | 3 +-- test/jQueryPatchSpec.js | 2 +- test/jqLiteSpec.js | 25 +++++++++++++++++-- test/ng/compileSpec.js | 8 +++--- test/ng/directive/formSpec.js | 2 +- test/ng/directive/ngIncludeSpec.js | 8 +++--- test/ngAnimate/animateSpec.js | 2 +- test/ngScenario/dslSpec.js | 2 +- test/ngTouch/directive/ngClickSpec.js | 2 +- 18 files changed, 65 insertions(+), 31 deletions(-) diff --git a/docs/component-spec/annotationsSpec.js b/docs/component-spec/annotationsSpec.js index a2a780779fee..60b17d9ad36d 100644 --- a/docs/component-spec/annotationsSpec.js +++ b/docs/component-spec/annotationsSpec.js @@ -5,7 +5,7 @@ describe('Docs Annotations', function() { var body; beforeEach(function() { body = angular.element(document.body); - body.html(''); + body.empty(); }); var normalizeHtml = function(html) { diff --git a/docs/components/angular-bootstrap/bootstrap-prettify.js b/docs/components/angular-bootstrap/bootstrap-prettify.js index 43248943854a..72c136d1c8f0 100644 --- a/docs/components/angular-bootstrap/bootstrap-prettify.js +++ b/docs/components/angular-bootstrap/bootstrap-prettify.js @@ -28,7 +28,7 @@ function escape(text) { function setHtmlIe8SafeWay(element, html) { var newElement = angular.element('
' + html + '
'); - element.html(''); + element.empty(); element.append(newElement.contents()); return element; } diff --git a/docs/content/guide/dev_guide.unit-testing.ngdoc b/docs/content/guide/dev_guide.unit-testing.ngdoc index 9d3d976546fa..e15edeb7cd20 100644 --- a/docs/content/guide/dev_guide.unit-testing.ngdoc +++ b/docs/content/guide/dev_guide.unit-testing.ngdoc @@ -222,7 +222,7 @@ var pc = new PasswordCtrl(); input.val('abc'); pc.grade(); expect(span.text()).toEqual('weak'); -$('body').html(''); +$('body').empty(); In angular the controllers are strictly separated from the DOM manipulation logic and this results in diff --git a/src/Angular.js b/src/Angular.js index 4faeb8d72eed..64b81bcc7e17 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -974,7 +974,7 @@ function startingTag(element) { try { // turns out IE does not let you set .html() on elements which // are not allowed to have children. So we just ignore it. - element.html(''); + element.empty(); } catch(e) {} // As Per DOM Standards var TEXT_NODE = 3; diff --git a/src/jqLite.js b/src/jqLite.js index e7531fb36379..8a45f9663d4e 100644 --- a/src/jqLite.js +++ b/src/jqLite.js @@ -46,6 +46,7 @@ * - [`contents()`](http://api.jquery.com/contents/) * - [`css()`](http://api.jquery.com/css/) * - [`data()`](http://api.jquery.com/data/) + * - [`empty()`](http://api.jquery.com/empty/) * - [`eq()`](http://api.jquery.com/eq/) * - [`find()`](http://api.jquery.com/find/) - Limited to lookups by tag name * - [`hasClass()`](http://api.jquery.com/hasClass/) @@ -358,6 +359,15 @@ function jqLiteInheritedData(element, name, value) { } } +function jqLiteEmpty(element) { + for (var i = 0, childNodes = element.childNodes; i < childNodes.length; i++) { + jqLiteDealoc(childNodes[i]); + } + while (element.firstChild) { + element.removeChild(element.firstChild); + } +} + ////////////////////////////////////////// // Functions which are declared directly. ////////////////////////////////////////// @@ -552,7 +562,9 @@ forEach({ jqLiteDealoc(childNodes[i]); } element.innerHTML = value; - } + }, + + empty: jqLiteEmpty }, function(fn, name){ /** * Properties: writes return selection, reads return first value @@ -562,11 +574,13 @@ forEach({ // jqLiteHasClass has only two arguments, but is a getter-only fn, so we need to special-case it // in a way that survives minification. - if (((fn.length == 2 && (fn !== jqLiteHasClass && fn !== jqLiteController)) ? arg1 : arg2) === undefined) { + // jqLiteEmpty takes no arguments but is a setter. + if (fn !== jqLiteEmpty && + (((fn.length == 2 && (fn !== jqLiteHasClass && fn !== jqLiteController)) ? arg1 : arg2) === undefined)) { if (isObject(arg1)) { // we are a write, but the object properties are the key/values - for(i=0; i < this.length; i++) { + for (i = 0; i < this.length; i++) { if (fn === jqLiteData) { // data() takes the whole object in jQuery fn(this[i], arg1); @@ -591,7 +605,7 @@ forEach({ } } else { // we are a write, so apply to all children - for(i=0; i < this.length; i++) { + for (i = 0; i < this.length; i++) { fn(this[i], arg1, arg2); } // return self for chaining diff --git a/src/ng/compile.js b/src/ng/compile.js index 7d0bb008db21..72e58fe73b24 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1219,7 +1219,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { }); } else { $template = jqLite(jqLiteClone(compileNode)).contents(); - $compileNode.html(''); // clear contents + $compileNode.empty(); // clear contents childTranscludeFn = compile($template, transcludeFn); } } @@ -1647,7 +1647,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { ? origAsyncDirective.templateUrl($compileNode, tAttrs) : origAsyncDirective.templateUrl; - $compileNode.html(''); + $compileNode.empty(); $http.get($sce.getTrustedResourceUrl(templateUrl), {cache: $templateCache}). success(function(content) { diff --git a/src/ng/directive/ngTransclude.js b/src/ng/directive/ngTransclude.js index 490ea21f2587..8eefb6fff528 100644 --- a/src/ng/directive/ngTransclude.js +++ b/src/ng/directive/ngTransclude.js @@ -69,7 +69,7 @@ var ngTranscludeDirective = ngDirective({ link: function($scope, $element, $attrs, controller) { controller.$transclude(function(clone) { - $element.html(''); + $element.empty(); $element.append(clone); }); } diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 86e042426c1b..d87fa5d3e0a4 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -333,13 +333,13 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { // becomes the compilation root nullOption.removeClass('ng-scope'); - // we need to remove it before calling selectElement.html('') because otherwise IE will + // we need to remove it before calling selectElement.empty() because otherwise IE will // remove the label from the element. wtf? nullOption.remove(); } // clear contents, we'll add what's needed based on the model - selectElement.html(''); + selectElement.empty(); selectElement.on('change', function() { scope.$apply(function() { diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index 8ff7b4298725..aeb6e32e9361 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -1237,7 +1237,7 @@ angular.module('ngAnimate', ['ng']) //make the element super hidden and override any CSS style values clone.attr('style','position:absolute; top:-9999px; left:-9999px'); clone.removeAttr('id'); - clone.html(''); + clone.empty(); forEach(oldClasses.split(' '), function(klass) { clone.removeClass(klass); diff --git a/test/helpers/testabilityPatch.js b/test/helpers/testabilityPatch.js index 41b5042ab273..f61be4eeb13f 100644 --- a/test/helpers/testabilityPatch.js +++ b/test/helpers/testabilityPatch.js @@ -29,8 +29,7 @@ beforeEach(function() { bindJQuery(); } - - angular.element(document.body).html('').removeData(); + angular.element(document.body).empty().removeData(); }); afterEach(function() { diff --git a/test/jQueryPatchSpec.js b/test/jQueryPatchSpec.js index 0efbb8d1ff83..98c666355ed7 100644 --- a/test/jQueryPatchSpec.js +++ b/test/jQueryPatchSpec.js @@ -54,7 +54,7 @@ if (window.jQuery) { }); it('should fire on html(\'\')', function() { - doc.html(''); + doc.empty(); }); }); }); diff --git a/test/jqLiteSpec.js b/test/jqLiteSpec.js index 09be1c1cb436..c4f47dcd2f55 100644 --- a/test/jqLiteSpec.js +++ b/test/jqLiteSpec.js @@ -322,7 +322,7 @@ describe('jqLite', function() { }); - it('should emit $destroy event if an element is removed via html()', inject(function(log) { + it('should emit $destroy event if an element is removed via html(\'\')', inject(function(log) { var element = jqLite('
x
'); element.find('span').on('$destroy', log.fn('destroyed')); @@ -333,6 +333,17 @@ describe('jqLite', function() { })); + it('should emit $destroy event if an element is removed via empty()', inject(function(log) { + var element = jqLite('
x
'); + element.find('span').on('$destroy', log.fn('destroyed')); + + element.empty(); + + expect(element.html()).toBe(''); + expect(log).toEqual('destroyed'); + })); + + it('should retrieve all data if called without params', function() { var element = jqLite(a); expect(element.data()).toEqual({}); @@ -786,7 +797,7 @@ describe('jqLite', function() { }); - it('should read/write value', function() { + it('should read/write a value', function() { var element = jqLite('
abc
'); expect(element.length).toEqual(1); expect(element[0].innerHTML).toEqual('abc'); @@ -797,6 +808,16 @@ describe('jqLite', function() { }); + describe('empty', function() { + it('should write a value', function() { + var element = jqLite('
abc
'); + expect(element.length).toEqual(1); + expect(element.empty() == element).toBeTruthy(); + expect(element.html()).toEqual(''); + }); + }); + + describe('on', function() { it('should bind to window on hashchange', function() { if (jqLite.fn) return; // don't run in jQuery diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index f2fa4ef66863..544e2024fe8d 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -170,26 +170,26 @@ describe('$compile', function() { // First with only elements at the top level element = jqLite('
'); $compile(element.contents())($rootScope); - element.html(''); + element.empty(); expect(calcCacheSize()).toEqual(0); // Next with non-empty text nodes at the top level // (in this case the compiler will wrap them in a ) element = jqLite('
xxx
'); $compile(element.contents())($rootScope); - element.html(''); + element.empty(); expect(calcCacheSize()).toEqual(0); // Next with comment nodes at the top level element = jqLite('
'); $compile(element.contents())($rootScope); - element.html(''); + element.empty(); expect(calcCacheSize()).toEqual(0); // Finally with empty text nodes at the top level element = jqLite('
\n
'); $compile(element.contents())($rootScope); - element.html(''); + element.empty(); expect(calcCacheSize()).toEqual(0); }); diff --git a/test/ng/directive/formSpec.js b/test/ng/directive/formSpec.js index 77beb2fd64ef..dde6f0a026c8 100644 --- a/test/ng/directive/formSpec.js +++ b/test/ng/directive/formSpec.js @@ -216,7 +216,7 @@ describe('form', function() { // yes, I know, scope methods should not do direct DOM manipulation, but I wanted to keep // this test small. Imagine that the destroy action will cause a model change (e.g. // $location change) that will cause some directive to destroy the dom (e.g. ngView+$route) - doc.html(''); + doc.empty(); destroyed = true; } diff --git a/test/ng/directive/ngIncludeSpec.js b/test/ng/directive/ngIncludeSpec.js index 2d115e8b683f..6247d4df01b3 100644 --- a/test/ng/directive/ngIncludeSpec.js +++ b/test/ng/directive/ngIncludeSpec.js @@ -47,7 +47,7 @@ describe('ngInclude', function() { $rootScope.url = 'myUrl'; $rootScope.$digest(); expect(body.text()).toEqual('misko'); - body.html(''); + body.empty(); })); @@ -60,7 +60,7 @@ describe('ngInclude', function() { $rootScope.url = 'myUrl'; $rootScope.$digest(); expect(element.text()).toEqual('Alibaba'); - jqLite(document.body).html(''); + jqLite(document.body).empty(); })); @@ -74,7 +74,7 @@ describe('ngInclude', function() { expect(function() { $rootScope.$digest(); }).toThrowMinErr( '$sce', 'insecurl', /Blocked loading resource from url not allowed by \$sceDelegate policy. URL: http:\/\/example.com\/myUrl.*/); - jqLite(document.body).html(''); + jqLite(document.body).empty(); })); @@ -88,7 +88,7 @@ describe('ngInclude', function() { expect(function() { $rootScope.$digest(); }).toThrowMinErr( '$sce', 'insecurl', /Blocked loading resource from url not allowed by \$sceDelegate policy. URL: http:\/\/example.com\/myUrl.*/); - jqLite(document.body).html(''); + jqLite(document.body).empty(); })); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index 44b623b43ef1..01589da19dde 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -301,7 +301,7 @@ describe("ngAnimate", function() { inject(function($animate, $compile, $rootScope, $timeout, $sniffer) { $rootScope.$digest(); - element.html(''); + element.empty(); var child1 = $compile('
1
')($rootScope); var child2 = $compile('
2
')($rootScope); diff --git a/test/ngScenario/dslSpec.js b/test/ngScenario/dslSpec.js index f94ec5831de5..2553fadd65b1 100644 --- a/test/ngScenario/dslSpec.js +++ b/test/ngScenario/dslSpec.js @@ -53,7 +53,7 @@ describe("angular.scenario.dsl", function() { // Just use the real one since it delegates to this.addFuture $root.addFutureAction = angular.scenario. SpecRunner.prototype.addFutureAction; - jqLite($window.document).html(''); + jqLite($window.document).empty(); })); afterEach(function(){ diff --git a/test/ngTouch/directive/ngClickSpec.js b/test/ngTouch/directive/ngClickSpec.js index 0eccc8f3d04b..43735709abea 100644 --- a/test/ngTouch/directive/ngClickSpec.js +++ b/test/ngTouch/directive/ngClickSpec.js @@ -152,7 +152,7 @@ describe('ngClick (touch)', function() { })); afterEach(inject(function($document) { - $document.find('body').html(''); + $document.find('body').empty(); }));