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

fix($timeout/$interval): do not trigger a digest when cancelling a $timeout/$interval #16064

Merged
merged 1 commit into from
Jul 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example of how this change can be "breaking" for some tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's probably not a breaking change? Should we mention this in the review anyway? (But no under Breaking Changes)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbedard Can you clarify how this can be "breaking"?

expect(function() {$timeout.verifyNoPendingTasks();}).not.toThrow();
$timeout.verifyNoPendingTasks();
}));

});
Expand Down