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

Commit f4fb6e0

Browse files
NarretzNarretz
committed
perf(*): don't trigger digests after 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). The `done` callback, which receives a single argument indicating success / failure was introduced to allow digest-less responses, but wasn't applied to these directives. Note that this applies to all calls to $animate.enter/leave, even if ngAnimate isn't included, and no actual animations are running, because the animation runner code is in the core ng module. Fixes #15322 Closes #15345
1 parent a18be15 commit f4fb6e0

File tree

8 files changed

+174
-18
lines changed

8 files changed

+174
-18
lines changed

‎src/ng/directive/ngIf.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,8 @@ var ngIfDirective = ['$animate', '$compile', function($animate, $compile) {
115115
}
116116
if (block) {
117117
previousElements = getBlockNodes(block.clone);
118-
$animate.leave(previousElements).then(function() {
119-
previousElements = null;
118+
$animate.leave(previousElements).done(function(response) {
119+
if (response !== false) previousElements = null;
120120
});
121121
block = null;
122122
}

‎src/ng/directive/ngInclude.js

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -214,18 +214,19 @@ var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate',
214214
currentScope = null;
215215
}
216216
if (currentElement) {
217-
$animate.leave(currentElement).then(function() {
218-
previousElement = null;
217+
$animate.leave(currentElement).done(function(response) {
218+
if (response !== false) previousElement = null;
219219
});
220220
previousElement = currentElement;
221221
currentElement = null;
222222
}
223223
};
224224

225225
scope.$watch(srcExp, function ngIncludeWatchAction(src) {
226-
var afterAnimation = function() {
227-
if (isDefined(autoScrollExp) && (!autoScrollExp || scope.$eval(autoScrollExp))) {
228-
$anchorScroll();
226+
var afterAnimation = function(response) {
227+
if (response !== false && isDefined(autoScrollExp) &&
228+
(!autoScrollExp || scope.$eval(autoScrollExp))) {
229+
$anchorScroll();
229230
}
230231
};
231232
var thisChangeId = ++changeCounter;
@@ -248,7 +249,7 @@ var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate',
248249
// directives to non existing elements.
249250
var clone = $transclude(newScope, function(clone) {
250251
cleanupLastIncludeContent();
251-
$animate.enter(clone, null, $element).then(afterAnimation);
252+
$animate.enter(clone, null, $element).done(afterAnimation);
252253
});
253254

254255
currentScope = newScope;

‎src/ng/directive/ngSwitch.js

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -153,21 +153,24 @@ 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(response) {
157+
if (response !== false) 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) {
162-
$animate.cancel(previousLeaveAnimations[i]);
163+
164+
// Start with the last, in case the array is modified during the loop
165+
while (previousLeaveAnimations.length) {
166+
$animate.cancel(previousLeaveAnimations.pop());
163167
}
164-
previousLeaveAnimations.length = 0;
165168

166169
for (i = 0, ii = selectedScopes.length; i < ii; ++i) {
167170
var selected = getBlockNodes(selectedElements[i].clone);
168171
selectedScopes[i].$destroy();
169-
var promise = previousLeaveAnimations[i] = $animate.leave(selected);
170-
promise.then(spliceFactory(previousLeaveAnimations, i));
172+
var runner = previousLeaveAnimations[i] = $animate.leave(selected);
173+
runner.done(spliceFactory(previousLeaveAnimations, i));
171174
}
172175

173176
selectedElements.length = 0;

‎src/ngRoute/directive/ngView.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -207,8 +207,8 @@ function ngViewFactory($route, $anchorScroll, $animate) {
207207
}
208208
if (currentElement) {
209209
previousLeaveAnimation = $animate.leave(currentElement);
210-
previousLeaveAnimation.then(function() {
211-
previousLeaveAnimation = null;
210+
previousLeaveAnimation.done(function(response) {
211+
if (response !== false) previousLeaveAnimation = null;
212212
});
213213
currentElement = null;
214214
}
@@ -229,8 +229,8 @@ 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() {
233-
if (angular.isDefined(autoScrollExp)
232+
$animate.enter(clone, null, currentElement || $element).done(function onNgViewEnter(response) {
233+
if (response !== false && angular.isDefined(autoScrollExp)
234234
&& (!autoScrollExp || scope.$eval(autoScrollExp))) {
235235
$anchorScroll();
236236
}

‎test/ng/directive/ngIfSpec.js

Lines changed: 18 additions & 0 deletions
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

Lines changed: 29 additions & 0 deletions
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

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,14 @@ describe('ngSwitch', function() {
5454
$rootScope.name = 'shyam';
5555
$rootScope.$apply();
5656
expect(element.text()).toEqual('first:shyam, first too:shyam');
57+
5758
$rootScope.select = 2;
5859
$rootScope.$apply();
5960
expect(element.text()).toEqual('second:shyam');
6061
$rootScope.name = 'misko';
6162
$rootScope.$apply();
6263
expect(element.text()).toEqual('second:misko');
64+
6365
$rootScope.select = true;
6466
$rootScope.$apply();
6567
expect(element.text()).toEqual('true:misko');
@@ -301,7 +303,66 @@ describe('ngSwitch', function() {
301303
}));
302304

303305

306+
it('should not trigger a digest after an element is removed', inject(function($$rAF, $compile, $rootScope, $timeout) {
307+
var spy = spyOn($rootScope, '$digest').and.callThrough();
308+
309+
$rootScope.select = 1;
310+
element = $compile(
311+
'<div ng-switch="select">' +
312+
'<div ng-switch-when="1">first</div>' +
313+
'<div ng-switch-when="2">second</div>' +
314+
'</div>')($rootScope);
315+
$rootScope.$apply();
316+
317+
expect(element.text()).toEqual('first');
318+
319+
$rootScope.select = 2;
320+
$rootScope.$apply();
321+
spy.calls.reset();
322+
expect(element.text()).toEqual('second');
323+
// If ngSwitch re-introduces code that triggers a digest after an element is removed (in an
324+
// animation .then callback), flushing the queue ensures the callback will be called, and the test
325+
// fails
326+
$$rAF.flush();
327+
328+
expect(spy).not.toHaveBeenCalled();
329+
// A digest may have been triggered asynchronously, so check the queue
330+
$timeout.verifyNoPendingTasks();
331+
}));
332+
333+
334+
it('should handle changes to the switch value in a digest loop with multiple value matches',
335+
inject(function($compile, $rootScope) {
336+
var scope = $rootScope.$new();
337+
scope.value = 'foo';
338+
339+
scope.$watch('value', function() {
340+
if (scope.value === 'bar') {
341+
scope.$evalAsync(function() {
342+
scope.value = 'baz';
343+
});
344+
}
345+
});
346+
347+
element = $compile(
348+
'<div ng-switch="value">' +
349+
'<div ng-switch-when="foo">FOO 1</div>' +
350+
'<div ng-switch-when="foo">FOO 2</div>' +
351+
'<div ng-switch-when="bar">BAR</div>' +
352+
'<div ng-switch-when="baz">BAZ</div>' +
353+
'</div>')(scope);
354+
355+
scope.$apply();
356+
expect(element.text()).toBe('FOO 1FOO 2');
357+
358+
scope.$apply('value = "bar"');
359+
expect(element.text()).toBe('BAZ');
360+
})
361+
);
362+
363+
304364
describe('ngSwitchWhen separator', function() {
365+
305366
it('should be possible to define a separator', inject(function($rootScope, $compile) {
306367
element = $compile(
307368
'<div ng-switch="mode">' +
@@ -315,9 +376,11 @@ describe('ngSwitch', function() {
315376
expect(element.children().length).toBe(2);
316377
expect(element.text()).toBe('Block1|Block2|');
317378
$rootScope.$apply('mode = "b"');
379+
318380
expect(element.children().length).toBe(1);
319381
expect(element.text()).toBe('Block1|');
320382
$rootScope.$apply('mode = "c"');
383+
321384
expect(element.children().length).toBe(1);
322385
expect(element.text()).toBe('Block3|');
323386
}));
@@ -336,9 +399,11 @@ describe('ngSwitch', function() {
336399
expect(element.children().length).toBe(2);
337400
expect(element.text()).toBe('Block1|Block2|');
338401
$rootScope.$apply('mode = ""');
402+
339403
expect(element.children().length).toBe(1);
340404
expect(element.text()).toBe('Block1|');
341405
$rootScope.$apply('mode = "c"');
406+
342407
expect(element.children().length).toBe(1);
343408
expect(element.text()).toBe('Block3|');
344409
}));
@@ -357,9 +422,11 @@ describe('ngSwitch', function() {
357422
expect(element.children().length).toBe(2);
358423
expect(element.text()).toBe('Block1|Block2|');
359424
$rootScope.$apply('mode = "b"');
425+
360426
expect(element.children().length).toBe(1);
361427
expect(element.text()).toBe('Block1|');
362428
$rootScope.$apply('mode = "c"');
429+
363430
expect(element.children().length).toBe(1);
364431
expect(element.text()).toBe('Block3|');
365432
}));
@@ -378,9 +445,11 @@ describe('ngSwitch', function() {
378445
expect(element.children().length).toBe(2);
379446
expect(element.text()).toBe('Block1|Block2|');
380447
$rootScope.$apply('mode = "b|a"');
448+
381449
expect(element.children().length).toBe(1);
382450
expect(element.text()).toBe('Block1|');
383451
$rootScope.$apply('mode = "c"');
452+
384453
expect(element.children().length).toBe(1);
385454
expect(element.text()).toBe('Block3|');
386455
}));
@@ -399,9 +468,11 @@ describe('ngSwitch', function() {
399468
expect(element.children().length).toBe(2);
400469
expect(element.text()).toBe('Block1|Block2|');
401470
$rootScope.$apply('mode = "b"');
471+
402472
expect(element.children().length).toBe(1);
403473
expect(element.text()).toBe('Block1|');
404474
$rootScope.$apply('mode = "c"');
475+
405476
expect(element.children().length).toBe(1);
406477
expect(element.text()).toBe('Block3|');
407478
}));

‎test/ngRoute/directive/ngViewSpec.js

Lines changed: 34 additions & 0 deletions
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)