Skip to content

Commit

Permalink
fix($timeout/$interval): do not trigger a digest on cancel
Browse files Browse the repository at this point in the history
Previously, `.catch(noop)` was used on a rejected timeout/interval to prevent an unhandled rejection error (introduced in #c9dffde1cb). However this would schedule a deferred task to run the `noop`. If the cancelling was outside a digest this could cause a new digest such as with the ng-model `debounce` option.

For unit testing, this means that it's no longer necessary to use `$timeout.flush()` when a `$timeout` has been cancelled outside of a digest. Previously, this was necessary to execute the deferred task added by `.catch(noop).
There's an example of such a change in this commit's changeset in the file `/test/ngAnimate/animateCssSpec.js`.

Fixes angular#16057
Closes angular#16064
  • Loading branch information
jbedard authored and Narretz committed Jul 3, 2017
1 parent bf0af6d commit cdaa6a9
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 8 deletions.
3 changes: 3 additions & 0 deletions src/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@
/* ng/compile.js */
"directiveNormalize": false,

/* ng/q.js */
"markQExceptionHandled": false,

/* ng/directive/directives.js */
"ngDirective": false,

Expand Down
2 changes: 1 addition & 1 deletion src/ng/interval.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ function $IntervalProvider() {
interval.cancel = function(promise) {
if (promise && promise.$$intervalId in intervals) {
// Interval cancels should not report as unhandled promise.
intervals[promise.$$intervalId].promise.catch(noop);
markQExceptionHandled(intervals[promise.$$intervalId].promise);
intervals[promise.$$intervalId].reject('canceled');
$window.clearInterval(promise.$$intervalId);
delete intervals[promise.$$intervalId];
Expand Down
18 changes: 14 additions & 4 deletions src/ng/q.js
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
state.pending = undefined;
try {
for (var i = 0, ii = pending.length; i < ii; ++i) {
state.pur = true;
markQStateExceptionHandled(state);
promise = pending[i][0];
fn = pending[i][state.status];
try {
Expand All @@ -378,8 +378,8 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
// eslint-disable-next-line no-unmodified-loop-condition
while (!queueSize && checkQueue.length) {
var toCheck = checkQueue.shift();
if (!toCheck.pur) {
toCheck.pur = true;
if (!isStateExceptionHandled(toCheck)) {
markQStateExceptionHandled(toCheck);
var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
if (isError(toCheck.value)) {
exceptionHandler(toCheck.value, errorMessage);
Expand All @@ -391,7 +391,7 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
}

function scheduleProcessQueue(state) {
if (errorOnUnhandledRejections && !state.pending && state.status === 2 && !state.pur) {
if (errorOnUnhandledRejections && !state.pending && state.status === 2 && !isStateExceptionHandled(state)) {
if (queueSize === 0 && checkQueue.length === 0) {
nextTick(processChecks);
}
Expand Down Expand Up @@ -671,3 +671,13 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {

return $Q;
}

function isStateExceptionHandled(state) {
return !!state.pur;
}
function markQStateExceptionHandled(state) {
state.pur = true;
}
function markQExceptionHandled(q) {
markQStateExceptionHandled(q.$$state);
}
2 changes: 1 addition & 1 deletion src/ng/timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ function $TimeoutProvider() {
timeout.cancel = function(promise) {
if (promise && promise.$$timeoutId in deferreds) {
// Timeout cancels should not report an unhandled promise.
deferreds[promise.$$timeoutId].promise.catch(noop);
markQExceptionHandled(deferreds[promise.$$timeoutId].promise);
deferreds[promise.$$timeoutId].reject('canceled');
delete deferreds[promise.$$timeoutId];
return $browser.defer.cancel(promise.$$timeoutId);
Expand Down
6 changes: 6 additions & 0 deletions test/ng/directive/ngModelOptionsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -459,8 +459,14 @@ describe('ngModelOptions', function() {
$rootScope.$watch(watchSpy);

helper.changeInputValueTo('a');
$timeout.flush(2000);
expect(watchSpy).not.toHaveBeenCalled();

helper.changeInputValueTo('b');
$timeout.flush(2000);
expect(watchSpy).not.toHaveBeenCalled();

helper.changeInputValueTo('c');
$timeout.flush(10000);
expect(watchSpy).toHaveBeenCalled();
});
Expand Down
11 changes: 11 additions & 0 deletions test/ng/intervalSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,17 @@ describe('$interval', function() {
inject(function($interval) {
expect($interval.cancel()).toBe(false);
}));


it('should not trigger digest when cancelled', inject(function($interval, $rootScope, $browser) {
var watchSpy = jasmine.createSpy('watchSpy');
$rootScope.$watch(watchSpy);

var t = $interval();
$interval.cancel(t);
expect(function() {$browser.defer.flush();}).toThrowError('No deferred tasks to be flushed');
expect(watchSpy).not.toHaveBeenCalled();
}));
});

describe('$window delegation', function() {
Expand Down
11 changes: 11 additions & 0 deletions test/ng/timeoutSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -299,5 +299,16 @@ describe('$timeout', function() {
$timeout.cancel(promise);
expect(cancelSpy).toHaveBeenCalledOnce();
}));


it('should not trigger digest when cancelled', inject(function($timeout, $rootScope, $browser) {
var watchSpy = jasmine.createSpy('watchSpy');
$rootScope.$watch(watchSpy);

var t = $timeout();
$timeout.cancel(t);
expect(function() {$browser.defer.flush();}).toThrowError('No deferred tasks to be flushed');
expect(watchSpy).not.toHaveBeenCalled();
}));
});
});
3 changes: 1 addition & 2 deletions test/ngAnimate/animateCssSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1328,8 +1328,7 @@ describe('ngAnimate $animateCss', function() {
animator.end();

expect(element.data(ANIMATE_TIMER_KEY)).toBeUndefined();
$timeout.flush();
expect(function() {$timeout.verifyNoPendingTasks();}).not.toThrow();
$timeout.verifyNoPendingTasks();
}));

});
Expand Down

0 comments on commit cdaa6a9

Please sign in to comment.