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

Commit 220df23

Browse files
committed
perf(*): don't trigger additional digests on enter/leave of structural directives
ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView). Previously, they were using promise callbacks (`then`), which caused an unnessecary digest. Background: In bf0f550, animation promises where introduced instead of animation callbacks. These promises were however not tied to the digest cycle, so you had to manually call `$apply` inside them. This was changed in c8700f0, so animation promise callbacks would always trigger a `$digest`. This meant that several built-in directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude, ngView). Note that this applies to all calls to $animate.enter/leave, even if no actual animations are running, because the animation runner code is in the core ng module. Fixes #15322
1 parent 83519f5 commit 220df23

File tree

8 files changed

+120
-9
lines changed

8 files changed

+120
-9
lines changed

src/ng/directive/ngIf.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ var ngIfDirective = ['$animate', '$compile', function($animate, $compile) {
115115
}
116116
if (block) {
117117
previousElements = getBlockNodes(block.clone);
118-
$animate.leave(previousElements).then(function() {
118+
$animate.leave(previousElements).done(function() {
119119
previousElements = null;
120120
});
121121
block = null;

src/ng/directive/ngInclude.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate',
214214
currentScope = null;
215215
}
216216
if (currentElement) {
217-
$animate.leave(currentElement).then(function() {
217+
$animate.leave(currentElement).done(function() {
218218
previousElement = null;
219219
});
220220
previousElement = currentElement;
@@ -248,7 +248,7 @@ var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate',
248248
// directives to non existing elements.
249249
var clone = $transclude(newScope, function(clone) {
250250
cleanupLastIncludeContent();
251-
$animate.enter(clone, null, $element).then(afterAnimation);
251+
$animate.enter(clone, null, $element).done(afterAnimation);
252252
});
253253

254254
currentScope = newScope;

src/ng/directive/ngSwitch.js

+8-4
Original file line numberDiff line numberDiff line change
@@ -153,21 +153,25 @@ var ngSwitchDirective = ['$animate', '$compile', function($animate, $compile) {
153153
selectedScopes = [];
154154

155155
var spliceFactory = function(array, index) {
156-
return function() { array.splice(index, 1); };
156+
return function() {
157+
array.splice(index, 1);
158+
};
157159
};
158160

159161
scope.$watch(watchExpr, function ngSwitchWatchAction(value) {
160162
var i, ii;
161-
for (i = 0, ii = previousLeaveAnimations.length; i < ii; ++i) {
163+
164+
// Start from the end, in case the array is modified during the loop
165+
for (i = previousLeaveAnimations.length - 1; i >= 0; i--) {
162166
$animate.cancel(previousLeaveAnimations[i]);
163167
}
164168
previousLeaveAnimations.length = 0;
165169

166170
for (i = 0, ii = selectedScopes.length; i < ii; ++i) {
167171
var selected = getBlockNodes(selectedElements[i].clone);
168172
selectedScopes[i].$destroy();
169-
var promise = previousLeaveAnimations[i] = $animate.leave(selected);
170-
promise.then(spliceFactory(previousLeaveAnimations, i));
173+
var runner = previousLeaveAnimations[i] = $animate.leave(selected);
174+
runner.done(spliceFactory(previousLeaveAnimations, i));
171175
}
172176

173177
selectedElements.length = 0;

src/ngRoute/directive/ngView.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ function ngViewFactory($route, $anchorScroll, $animate) {
207207
}
208208
if (currentElement) {
209209
previousLeaveAnimation = $animate.leave(currentElement);
210-
previousLeaveAnimation.then(function() {
210+
previousLeaveAnimation.done(function() {
211211
previousLeaveAnimation = null;
212212
});
213213
currentElement = null;
@@ -229,7 +229,7 @@ function ngViewFactory($route, $anchorScroll, $animate) {
229229
// function is called before linking the content, which would apply child
230230
// directives to non existing elements.
231231
var clone = $transclude(newScope, function(clone) {
232-
$animate.enter(clone, null, currentElement || $element).then(function onNgViewEnter() {
232+
$animate.enter(clone, null, currentElement || $element).done(function onNgViewEnter() {
233233
if (angular.isDefined(autoScrollExp)
234234
&& (!autoScrollExp || scope.$eval(autoScrollExp))) {
235235
$anchorScroll();

test/ng/directive/ngIfSpec.js

+18
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,23 @@ describe('ngIf', function() {
185185
expect(element.children().length).toBe(0);
186186
expect(element.text()).toBe('');
187187
}));
188+
189+
it('should not trigger a digest when the element is removed', inject(function($$rAF, $rootScope, $timeout) {
190+
var spy = spyOn($rootScope, '$digest').and.callThrough();
191+
192+
$scope.hello = true;
193+
makeIf('hello');
194+
expect(element.children().length).toBe(1);
195+
$scope.$apply('hello = false');
196+
spy.calls.reset();
197+
expect(element.children().length).toBe(0);
198+
// The animation completion is async even without actual animations
199+
$$rAF.flush();
200+
201+
expect(spy).not.toHaveBeenCalled();
202+
// A digest may have been triggered asynchronously, so check the queue
203+
$timeout.verifyNoPendingTasks();
204+
}));
188205
});
189206

190207
describe('and transcludes', function() {
@@ -319,6 +336,7 @@ describe('ngIf', function() {
319336
module(function($provide) {
320337
$provide.decorator('$animate', function($delegate, $$q) {
321338
var emptyPromise = $$q.defer().promise;
339+
emptyPromise.done = noop;
322340

323341
$delegate.leave = function() {
324342
return emptyPromise;

test/ng/directive/ngIncludeSpec.js

+29
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,34 @@ describe('ngInclude', function() {
423423
});
424424

425425

426+
it('should not trigger a digest when the include is changed', function() {
427+
428+
inject(function($$rAF, $templateCache, $rootScope, $compile, $timeout) {
429+
var spy = spyOn($rootScope, '$digest').and.callThrough();
430+
431+
$templateCache.put('myUrl', 'my template content');
432+
$templateCache.put('myOtherUrl', 'my other template content');
433+
434+
$rootScope.url = 'myUrl';
435+
element = jqLite('<div><ng-include src="url"></ng-include></div>');
436+
element = $compile(element)($rootScope);
437+
$rootScope.$digest();
438+
// The animation completion is async even without actual animations
439+
$$rAF.flush();
440+
expect(element.text()).toEqual('my template content');
441+
442+
$rootScope.$apply('url = "myOtherUrl"');
443+
spy.calls.reset();
444+
expect(element.text()).toEqual('my other template content');
445+
$$rAF.flush();
446+
447+
expect(spy).not.toHaveBeenCalled();
448+
// A digest may have been triggered asynchronously, so check the queue
449+
$timeout.verifyNoPendingTasks();
450+
});
451+
});
452+
453+
426454
describe('autoscroll', function() {
427455
var autoScrollSpy;
428456

@@ -759,6 +787,7 @@ describe('ngInclude', function() {
759787
module(function($provide) {
760788
$provide.decorator('$animate', function($delegate, $$q) {
761789
var emptyPromise = $$q.defer().promise;
790+
emptyPromise.done = noop;
762791

763792
$delegate.leave = function() {
764793
return emptyPromise;

test/ng/directive/ngSwitchSpec.js

+26
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,32 @@ describe('ngSwitch', function() {
301301
}));
302302

303303

304+
it('should not trigger a digest when an element is removed', inject(function($$rAF, $compile, $rootScope, $timeout) {
305+
var spy = spyOn($rootScope, '$digest').and.callThrough();
306+
307+
$rootScope.select = 1;
308+
element = $compile(
309+
'<div ng-switch="select">' +
310+
'<div ng-switch-when="1">first</div>' +
311+
'<div ng-switch-when="2">second</div>' +
312+
'</div>')($rootScope);
313+
$rootScope.$apply();
314+
315+
expect(element.text()).toEqual('first');
316+
317+
$rootScope.select = 2;
318+
$rootScope.$apply();
319+
spy.calls.reset();
320+
expect(element.text()).toEqual('second');
321+
// The animation completion is async even without actual animations
322+
$$rAF.flush();
323+
324+
expect(spy).not.toHaveBeenCalled();
325+
// A digest may have been triggered asynchronously, so check the queue
326+
$timeout.verifyNoPendingTasks();
327+
}));
328+
329+
304330
describe('ngSwitchWhen separator', function() {
305331
it('should be possible to define a separator', inject(function($rootScope, $compile) {
306332
element = $compile(

test/ngRoute/directive/ngViewSpec.js

+34
Original file line numberDiff line numberDiff line change
@@ -584,6 +584,40 @@ describe('ngView', function() {
584584
});
585585
});
586586
});
587+
588+
589+
it('should not trigger a digest when the view is changed', function() {
590+
module(function($routeProvider) {
591+
$routeProvider.when('/foo', {templateUrl: 'myUrl1'});
592+
$routeProvider.when('/bar', {templateUrl: 'myUrl2'});
593+
});
594+
595+
inject(function($$rAF, $templateCache, $rootScope, $compile, $timeout, $location, $httpBackend) {
596+
var spy = spyOn($rootScope, '$digest').and.callThrough();
597+
598+
$templateCache.put('myUrl1', 'my template content');
599+
$templateCache.put('myUrl2', 'my other template content');
600+
601+
$location.path('/foo');
602+
$rootScope.$digest();
603+
604+
// The animation completion is async even without actual animations
605+
$$rAF.flush();
606+
expect(element.text()).toEqual('my template content');
607+
608+
$location.path('/bar');
609+
$rootScope.$digest();
610+
spy.calls.reset();
611+
612+
$$rAF.flush();
613+
expect(element.text()).toEqual('my other template content');
614+
615+
expect(spy).not.toHaveBeenCalled();
616+
// A digest may have been triggered asynchronously, so check the queue
617+
$timeout.verifyNoPendingTasks();
618+
});
619+
});
620+
587621
});
588622

589623
describe('and transcludes', function() {

0 commit comments

Comments
 (0)