Skip to content
Permalink
Browse files

perf(*): don't trigger digests after enter/leave of structural direct…

…ives

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
  • Loading branch information...
Narretz authored and petebacondarwin committed Oct 29, 2016
1 parent 4040460 commit c57779d8725493c5853dceda0105dafd5c0e3a7c
@@ -115,8 +115,8 @@ var ngIfDirective = ['$animate', '$compile', function($animate, $compile) {
}
if (block) {
previousElements = getBlockNodes(block.clone);
$animate.leave(previousElements).then(function() {
previousElements = null;
$animate.leave(previousElements).done(function(response) {
if (response !== false) previousElements = null;
});
block = null;
}
@@ -214,18 +214,19 @@ var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate',
currentScope = null;
}
if (currentElement) {
$animate.leave(currentElement).then(function() {
previousElement = null;
$animate.leave(currentElement).done(function(response) {
if (response !== false) previousElement = null;
});
previousElement = currentElement;
currentElement = null;
}
};

scope.$watch(srcExp, function ngIncludeWatchAction(src) {
var afterAnimation = function() {
if (isDefined(autoScrollExp) && (!autoScrollExp || scope.$eval(autoScrollExp))) {
$anchorScroll();
var afterAnimation = function(response) {
if (response !== false && isDefined(autoScrollExp) &&
(!autoScrollExp || scope.$eval(autoScrollExp))) {
$anchorScroll();
}
};
var thisChangeId = ++changeCounter;
@@ -248,7 +249,7 @@ var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate',
// directives to non existing elements.
var clone = $transclude(newScope, function(clone) {
cleanupLastIncludeContent();
$animate.enter(clone, null, $element).then(afterAnimation);
$animate.enter(clone, null, $element).done(afterAnimation);
});

currentScope = newScope;
@@ -153,21 +153,24 @@ var ngSwitchDirective = ['$animate', '$compile', function($animate, $compile) {
selectedScopes = [];

var spliceFactory = function(array, index) {
return function() { array.splice(index, 1); };
return function(response) {
if (response !== false) array.splice(index, 1);
};
};

scope.$watch(watchExpr, function ngSwitchWatchAction(value) {
var i, ii;
for (i = 0, ii = previousLeaveAnimations.length; i < ii; ++i) {
$animate.cancel(previousLeaveAnimations[i]);

// Start with the last, in case the array is modified during the loop
while (previousLeaveAnimations.length) {
$animate.cancel(previousLeaveAnimations.pop());
}
previousLeaveAnimations.length = 0;

for (i = 0, ii = selectedScopes.length; i < ii; ++i) {
var selected = getBlockNodes(selectedElements[i].clone);
selectedScopes[i].$destroy();
var promise = previousLeaveAnimations[i] = $animate.leave(selected);
promise.then(spliceFactory(previousLeaveAnimations, i));
var runner = previousLeaveAnimations[i] = $animate.leave(selected);
runner.done(spliceFactory(previousLeaveAnimations, i));
}

selectedElements.length = 0;
@@ -214,8 +214,8 @@ function ngViewFactory($route, $anchorScroll, $animate) {
}
if (currentElement) {
previousLeaveAnimation = $animate.leave(currentElement);
previousLeaveAnimation.then(function() {
previousLeaveAnimation = null;
previousLeaveAnimation.done(function(response) {
if (response !== false) previousLeaveAnimation = null;
});
currentElement = null;
}
@@ -236,8 +236,8 @@ function ngViewFactory($route, $anchorScroll, $animate) {
// function is called before linking the content, which would apply child
// directives to non existing elements.
var clone = $transclude(newScope, function(clone) {
$animate.enter(clone, null, currentElement || $element).then(function onNgViewEnter() {
if (angular.isDefined(autoScrollExp)
$animate.enter(clone, null, currentElement || $element).done(function onNgViewEnter(response) {
if (response !== false && angular.isDefined(autoScrollExp)
&& (!autoScrollExp || scope.$eval(autoScrollExp))) {
$anchorScroll();
}
@@ -185,6 +185,23 @@ describe('ngIf', function() {
expect(element.children().length).toBe(0);
expect(element.text()).toBe('');
}));

it('should not trigger a digest when the element is removed', inject(function($$rAF, $rootScope, $timeout) {
var spy = spyOn($rootScope, '$digest').and.callThrough();

$scope.hello = true;
makeIf('hello');
expect(element.children().length).toBe(1);
$scope.$apply('hello = false');
spy.calls.reset();
expect(element.children().length).toBe(0);
// The animation completion is async even without actual animations
$$rAF.flush();

expect(spy).not.toHaveBeenCalled();
// A digest may have been triggered asynchronously, so check the queue
$timeout.verifyNoPendingTasks();
}));
});

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

$delegate.leave = function() {
return emptyPromise;
@@ -423,6 +423,34 @@ describe('ngInclude', function() {
});


it('should not trigger a digest when the include is changed', function() {

inject(function($$rAF, $templateCache, $rootScope, $compile, $timeout) {
var spy = spyOn($rootScope, '$digest').and.callThrough();

$templateCache.put('myUrl', 'my template content');
$templateCache.put('myOtherUrl', 'my other template content');

$rootScope.url = 'myUrl';
element = jqLite('<div><ng-include src="url"></ng-include></div>');
element = $compile(element)($rootScope);
$rootScope.$digest();
// The animation completion is async even without actual animations
$$rAF.flush();
expect(element.text()).toEqual('my template content');

$rootScope.$apply('url = "myOtherUrl"');
spy.calls.reset();
expect(element.text()).toEqual('my other template content');
$$rAF.flush();

expect(spy).not.toHaveBeenCalled();
// A digest may have been triggered asynchronously, so check the queue
$timeout.verifyNoPendingTasks();
});
});


describe('autoscroll', function() {
var autoScrollSpy;

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

$delegate.leave = function() {
return emptyPromise;
@@ -54,12 +54,14 @@ describe('ngSwitch', function() {
$rootScope.name = 'shyam';
$rootScope.$apply();
expect(element.text()).toEqual('first:shyam, first too:shyam');

$rootScope.select = 2;
$rootScope.$apply();
expect(element.text()).toEqual('second:shyam');
$rootScope.name = 'misko';
$rootScope.$apply();
expect(element.text()).toEqual('second:misko');

$rootScope.select = true;
$rootScope.$apply();
expect(element.text()).toEqual('true:misko');
@@ -301,7 +303,66 @@ describe('ngSwitch', function() {
}));


it('should not trigger a digest after an element is removed', inject(function($$rAF, $compile, $rootScope, $timeout) {
var spy = spyOn($rootScope, '$digest').and.callThrough();

$rootScope.select = 1;
element = $compile(
'<div ng-switch="select">' +
'<div ng-switch-when="1">first</div>' +
'<div ng-switch-when="2">second</div>' +
'</div>')($rootScope);
$rootScope.$apply();

expect(element.text()).toEqual('first');

$rootScope.select = 2;
$rootScope.$apply();
spy.calls.reset();
expect(element.text()).toEqual('second');
// If ngSwitch re-introduces code that triggers a digest after an element is removed (in an
// animation .then callback), flushing the queue ensures the callback will be called, and the test
// fails
$$rAF.flush();

expect(spy).not.toHaveBeenCalled();
// A digest may have been triggered asynchronously, so check the queue
$timeout.verifyNoPendingTasks();
}));


it('should handle changes to the switch value in a digest loop with multiple value matches',
inject(function($compile, $rootScope) {
var scope = $rootScope.$new();
scope.value = 'foo';

scope.$watch('value', function() {
if (scope.value === 'bar') {
scope.$evalAsync(function() {
scope.value = 'baz';
});
}
});

element = $compile(
'<div ng-switch="value">' +
'<div ng-switch-when="foo">FOO 1</div>' +
'<div ng-switch-when="foo">FOO 2</div>' +
'<div ng-switch-when="bar">BAR</div>' +
'<div ng-switch-when="baz">BAZ</div>' +
'</div>')(scope);

scope.$apply();
expect(element.text()).toBe('FOO 1FOO 2');

scope.$apply('value = "bar"');
expect(element.text()).toBe('BAZ');
})
);


describe('ngSwitchWhen separator', function() {

it('should be possible to define a separator', inject(function($rootScope, $compile) {
element = $compile(
'<div ng-switch="mode">' +
@@ -315,9 +376,11 @@ describe('ngSwitch', function() {
expect(element.children().length).toBe(2);
expect(element.text()).toBe('Block1|Block2|');
$rootScope.$apply('mode = "b"');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block1|');
$rootScope.$apply('mode = "c"');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block3|');
}));
@@ -336,9 +399,11 @@ describe('ngSwitch', function() {
expect(element.children().length).toBe(2);
expect(element.text()).toBe('Block1|Block2|');
$rootScope.$apply('mode = ""');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block1|');
$rootScope.$apply('mode = "c"');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block3|');
}));
@@ -357,9 +422,11 @@ describe('ngSwitch', function() {
expect(element.children().length).toBe(2);
expect(element.text()).toBe('Block1|Block2|');
$rootScope.$apply('mode = "b"');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block1|');
$rootScope.$apply('mode = "c"');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block3|');
}));
@@ -378,9 +445,11 @@ describe('ngSwitch', function() {
expect(element.children().length).toBe(2);
expect(element.text()).toBe('Block1|Block2|');
$rootScope.$apply('mode = "b|a"');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block1|');
$rootScope.$apply('mode = "c"');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block3|');
}));
@@ -399,9 +468,11 @@ describe('ngSwitch', function() {
expect(element.children().length).toBe(2);
expect(element.text()).toBe('Block1|Block2|');
$rootScope.$apply('mode = "b"');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block1|');
$rootScope.$apply('mode = "c"');

expect(element.children().length).toBe(1);
expect(element.text()).toBe('Block3|');
}));
@@ -581,6 +581,40 @@ describe('ngView', function() {
}
});
});


it('should not trigger a digest when the view is changed', function() {
module(function($routeProvider) {
$routeProvider.when('/foo', {templateUrl: 'myUrl1'});
$routeProvider.when('/bar', {templateUrl: 'myUrl2'});
});

inject(function($$rAF, $templateCache, $rootScope, $compile, $timeout, $location, $httpBackend) {
var spy = spyOn($rootScope, '$digest').and.callThrough();

$templateCache.put('myUrl1', 'my template content');
$templateCache.put('myUrl2', 'my other template content');

$location.path('/foo');
$rootScope.$digest();

// The animation completion is async even without actual animations
$$rAF.flush();
expect(element.text()).toEqual('my template content');

$location.path('/bar');
$rootScope.$digest();
spy.calls.reset();

$$rAF.flush();
expect(element.text()).toEqual('my other template content');

expect(spy).not.toHaveBeenCalled();
// A digest may have been triggered asynchronously, so check the queue
$timeout.verifyNoPendingTasks();
});
});

});
});

0 comments on commit c57779d

Please sign in to comment.
You can’t perform that action at this time.