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

Commit

Permalink
fix($interval): throw when trying to cancel non-$interval promise
Browse files Browse the repository at this point in the history
Previously, calling `$interval.cancel()` with a promise that was not
generated by a call to `$interval()` would do nothing. This could, for
example, happen when calling `.then()`/`.catch()` on the returned
promise, which creates a new promise, and passing that to
`$interval.cancel()`.
With this commit, `$interval.cancel()` will throw an error if called
with a non-$interval promise, thus surfacing errors that would otherwise
go unnoticed.

Related to #16424.

BREAKING CHNAGE:

`$interval.cancel()` will throw an error if called with a promise that
was not generated by `$interval()`. Previously, it would silently do
nothing.

Before:
```js
var promise = $interval(doSomething, 1000, 5).then(doSomethingElse);
$interval.cancel(promise);  // No error; interval NOT canceled.
```

After:
```js
var promise = $interval(doSomething, 1000, 5).then(doSomethingElse);
$interval.cancel(promise);  // Throws error.
```

Correct usage:
```js
var promise = $interval(doSomething, 1000, 5);
var newPromise = promise.then(doSomethingElse);
$interval.cancel(promise);  // Interval canceled.
```

Closes #16476
  • Loading branch information
gkalpak committed Mar 13, 2018
1 parent 3365256 commit a8bef95
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 10 deletions.
25 changes: 25 additions & 0 deletions docs/content/error/$interval/badprom.ngdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
@ngdoc error
@name $interval:badprom
@fullName Non-$interval promise
@description

This error occurs when calling {@link ng.$interval#cancel $interval.cancel()} with a promise that
was not generated by the {@link ng.$interval $interval} service. This can, for example, happen when
calling {@link ng.$q#the-promise-api then()/catch()} on the returned promise, which creates a new
promise, and pass that new promise to {@link ng.$interval#cancel $interval.cancel()}.

Example of incorrect usage that leads to this error:

```js
var promise = $interval(doSomething, 1000, 5).then(doSomethingElse);
$interval.cancel(promise);
```

To fix the example above, keep a reference to the promise returned by
{@link ng.$interval $interval()} and pass that to {@link ng.$interval#cancel $interval.cancel()}:

```js
var promise = $interval(doSomething, 1000, 5);
var newPromise = promise.then(doSomethingElse);
$interval.cancel(promise);
```
28 changes: 20 additions & 8 deletions src/ng/interval.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

var $intervalMinErr = minErr('$interval');

/** @this */
function $IntervalProvider() {
this.$get = ['$rootScope', '$window', '$q', '$$q', '$browser',
Expand Down Expand Up @@ -188,15 +190,25 @@ function $IntervalProvider() {
* @returns {boolean} Returns `true` if the task was successfully canceled.
*/
interval.cancel = function(promise) {
if (promise && promise.$$intervalId in intervals) {
// Interval cancels should not report as unhandled promise.
markQExceptionHandled(intervals[promise.$$intervalId].promise);
intervals[promise.$$intervalId].reject('canceled');
$window.clearInterval(promise.$$intervalId);
delete intervals[promise.$$intervalId];
return true;
if (!promise) return false;

if (!promise.hasOwnProperty('$$intervalId')) {
throw $intervalMinErr('badprom',
'`$interval.cancel()` called with a promise that was not generated by `$interval()`.');
}
return false;

if (!intervals.hasOwnProperty(promise.$$intervalId)) return false;

var id = promise.$$intervalId;
var deferred = intervals[id];

// Interval cancels should not report an unhandled promise.
markQExceptionHandled(deferred.promise);
deferred.reject('canceled');
$window.clearInterval(id);
delete intervals[id];

return true;
};

return interval;
Expand Down
9 changes: 7 additions & 2 deletions test/ng/intervalSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,17 @@ describe('$interval', function() {
}));


it('should not throw a runtime exception when given an undefined promise',
inject(function($interval) {
it('should not throw an error when given an undefined promise', inject(function($interval) {
expect($interval.cancel()).toBe(false);
}));


it('should throw an error when given a non-$interval promise', inject(function($interval) {
var promise = $interval(noop).then(noop);
expect(function() { $interval.cancel(promise); }).toThrowMinErr('$interval', 'badprom');
}));


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

0 comments on commit a8bef95

Please sign in to comment.