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

Commit

Permalink
feat($resource): add proper support for cancelling requests
Browse files Browse the repository at this point in the history
Introduced changes:

- Deprecate passing a promise as `timeout` (for `$resource` actions).
  It never worked correctly anyway.
  Now a warning is logged (using `$log.debug()`) and the property is
  removed.
- Add support for a boolean `cancellable` property in actions'
  configuration, the `$resource` factory's `options` parameter and the
  `$resourceProvider`'s `defaults` property.
  If true, the `$cancelRequest` method (added to all returned values for
  non-instance calls) will abort the request (if it's not already
  completed or aborted).
  If there is a numeric `timeout` specified on the action's configuration,
  the value of `cancellable` will be ignored.

Example usage:

```js
var Post = $resource('/posts/:id', {id: '@id'}, {
  get: {
    method: 'GET',
    cancellable: true
  }
});

var currentPost = Post.get({id: 1});
...
// A moment later the user selects another post, so
// we don't need the previous request any more
currentPost.$cancelRequest();
currentPost = Post.get({id: 2});
...
```

BREAKING CHANGE:

Using a promise as `timeout` is no longer supported and will log a
warning. It never worked the way it was supposed to anyway.

Before:

```js
var deferred = $q.defer();
var User = $resource('/api/user/:id', {id: '@id'}, {
  get: {method: 'GET', timeout: deferred.promise}
});

var user = User.get({id: 1});   // sends a request
deferred.resolve();             // aborts the request

// Now, we need to re-define `User` passing a new promise as `timeout`
// or else all subsequent requests from `someAction` will be aborted
User = $resource(...);
user = User.get({id: 2});
```

After:

```js
var User = $resource('/api/user/:id', {id: '@id'}, {
  get: {method: 'GET', cancellable: true}
});

var user = User.get({id: 1});   // sends a request
instance.$cancelRequest();      // aborts the request

user = User.get({id: 2});
```

Fixes #9332
Closes #13050
Closes #13058
Closes #13210
  • Loading branch information
gkalpak authored and petebacondarwin committed Nov 24, 2015
1 parent 9190d4c commit 98528be
Show file tree
Hide file tree
Showing 2 changed files with 296 additions and 43 deletions.
135 changes: 105 additions & 30 deletions src/ngResource/resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ function shallowClearAndCopy(src, dst) {
* @ngdoc service
* @name $resource
* @requires $http
* @requires ng.$log
* @requires $q
*
* @description
* A factory which creates a resource object that lets you interact with
Expand Down Expand Up @@ -107,9 +109,9 @@ function shallowClearAndCopy(src, dst) {
* URL `/path/greet?salutation=Hello`.
*
* If the parameter value is prefixed with `@` then the value for that parameter will be extracted
* from the corresponding property on the `data` object (provided when calling an action method). For
* example, if the `defaultParam` object is `{someParam: '@someProp'}` then the value of `someParam`
* will be `data.someProp`.
* from the corresponding property on the `data` object (provided when calling an action method).
* For example, if the `defaultParam` object is `{someParam: '@someProp'}` then the value of
* `someParam` will be `data.someProp`.
*
* @param {Object.<Object>=} actions Hash with declaration of custom actions that should extend
* the default set of resource actions. The declaration should be created in the format of {@link
Expand Down Expand Up @@ -143,15 +145,23 @@ function shallowClearAndCopy(src, dst) {
* `{function(data, headersGetter)|Array.<function(data, headersGetter)>}` –
* transform function or an array of such functions. The transform function takes the http
* response body and headers and returns its transformed (typically deserialized) version.
* By default, transformResponse will contain one function that checks if the response looks like
* a JSON string and deserializes it using `angular.fromJson`. To prevent this behavior, set
* `transformResponse` to an empty array: `transformResponse: []`
* By default, transformResponse will contain one function that checks if the response looks
* like a JSON string and deserializes it using `angular.fromJson`. To prevent this behavior,
* set `transformResponse` to an empty array: `transformResponse: []`
* - **`cache`** – `{boolean|Cache}` – If true, a default $http cache will be used to cache the
* GET request, otherwise if a cache instance built with
* {@link ng.$cacheFactory $cacheFactory}, this cache will be used for
* caching.
* - **`timeout`** – `{number|Promise}` – timeout in milliseconds, or {@link ng.$q promise} that
* should abort the request when resolved.
* - **`timeout`** – `{number}` – timeout in milliseconds.<br />
* **Note:** In contrast to {@link ng.$http#usage $http.config}, {@link ng.$q promises} are
* **not** supported in $resource, because the same value has to be re-used for multiple
* requests. If you are looking for a way to cancel requests, you should use the `cancellable`
* option.
* - **`cancellable`** – `{boolean}` – if set to true, the request made by a "non-instance" call
* will be cancelled (if not already completed) by calling `$cancelRequest()` on the call's
* return value. Calling `$cancelRequest()` for a non-cancellable or an already
* completed/cancelled request will have no effect.<br />
* **Note:** If a timeout is specified in millisecondes, `cancellable` is ignored.
* - **`withCredentials`** - `{boolean}` - whether to set the `withCredentials` flag on the
* XHR object. See
* [requests with credentials](https://developer.mozilla.org/en/http_access_control#section_5)
Expand All @@ -163,12 +173,13 @@ function shallowClearAndCopy(src, dst) {
* with `http response` object. See {@link ng.$http $http interceptors}.
*
* @param {Object} options Hash with custom settings that should extend the
* default `$resourceProvider` behavior. The only supported option is
*
* Where:
* default `$resourceProvider` behavior. The supported options are:
*
* - **`stripTrailingSlashes`** – {boolean} – If true then the trailing
* slashes from any calculated URL will be stripped. (Defaults to true.)
* - **`cancellable`** – {boolean} – If true, the request made by a "non-instance" call will be
* cancelled (if not already completed) by calling `$cancelRequest()` on the call's return value.
* This can be overwritten per action. (Defaults to false.)
*
* @returns {Object} A resource "class" object with methods for the default set of resource actions
* optionally extended with custom `actions`. The default set contains these actions:
Expand Down Expand Up @@ -216,7 +227,7 @@ function shallowClearAndCopy(src, dst) {
* Class actions return empty instance (with additional properties below).
* Instance actions return promise of the action.
*
* The Resource instances and collection have these additional properties:
* The Resource instances and collections have these additional properties:
*
* - `$promise`: the {@link ng.$q promise} of the original server interaction that created this
* instance or collection.
Expand All @@ -236,6 +247,11 @@ function shallowClearAndCopy(src, dst) {
* rejection), `false` before that. Knowing if the Resource has been resolved is useful in
* data-binding.
*
* The Resource instances and collections have these additional methods:
*
* - `$cancelRequest`: If there is a cancellable, pending request related to the instance or
* collection, calling this method will abort the request.
*
* @example
*
* # Credit card resource
Expand Down Expand Up @@ -280,6 +296,11 @@ function shallowClearAndCopy(src, dst) {
*
* Calling these methods invoke `$http` on the `url` template with the given `method`, `params` and
* `headers`.
*
* @example
*
* # User resource
*
* When the data is returned from the server then the object is an instance of the resource type and
* all of the non-GET methods are available with `$` prefix. This allows you to easily support CRUD
* operations (create, read, update, delete) on server-side data.
Expand All @@ -298,10 +319,10 @@ function shallowClearAndCopy(src, dst) {
*
```js
var User = $resource('/user/:userId', {userId:'@id'});
User.get({userId:123}, function(u, getResponseHeaders){
u.abc = true;
u.$save(function(u, putResponseHeaders) {
//u => saved user object
User.get({userId:123}, function(user, getResponseHeaders){
user.abc = true;
user.$save(function(user, putResponseHeaders) {
//user => saved user object
//putResponseHeaders => $http header getter
});
});
Expand All @@ -316,8 +337,11 @@ function shallowClearAndCopy(src, dst) {
$scope.user = user;
});
```
*
* @example
*
* # Creating a custom 'PUT' request
*
* In this example we create a custom method on our resource to make a PUT request
* ```js
* var app = angular.module('app', ['ngResource', 'ngRoute']);
Expand Down Expand Up @@ -345,6 +369,34 @@ function shallowClearAndCopy(src, dst) {
* // This will PUT /notes/ID with the note object in the request payload
* }]);
* ```
*
* @example
*
* # Cancelling requests
*
* If an action's configuration specifies that it is cancellable, you can cancel the request related
* to an instance or collection (as long as it is a result of a "non-instance" call):
*
```js
// ...defining the `Hotel` resource...
var Hotel = $resource('/api/hotel/:id', {id: '@id'}, {
// Let's make the `query()` method cancellable
query: {method: 'get', isArray: true, cancellable: true}
});
// ...somewhere in the PlanVacationController...
...
this.onDestinationChanged = function onDestinationChanged(destination) {
// We don't care about any pending request for hotels
// in a different destination any more
this.availableHotels.$cancelRequest();
// Let's query for hotels in '<destination>'
// (calls: /api/hotel?location=<destination>)
this.availableHotels = Hotel.query({location: destination});
};
```
*
*/
angular.module('ngResource', ['ng']).
provider('$resource', function() {
Expand All @@ -365,7 +417,7 @@ angular.module('ngResource', ['ng']).
}
};

this.$get = ['$http', '$q', function($http, $q) {
this.$get = ['$http', '$log', '$q', function($http, $log, $q) {

var noop = angular.noop,
forEach = angular.forEach,
Expand Down Expand Up @@ -524,6 +576,22 @@ angular.module('ngResource', ['ng']).

forEach(actions, function(action, name) {
var hasBody = /^(POST|PUT|PATCH)$/i.test(action.method);
var cancellable;

if (angular.isNumber(action.timeout)) {
cancellable = false;
} else if (action.timeout) {
$log.debug('ngResource:\n' +
' Only numeric values are allowed as `timeout`.\n' +
' Promises are not supported in $resource, because the same value has to ' +
'be re-used for multiple requests. If you are looking for a way to cancel ' +
'requests, you should use the `cancellable` option.');
delete action.timeout;
} else {
cancellable = angular.isDefined(action.cancellable) ? action.cancellable :
(options && angular.isDefined(options.cancellable)) ? options.cancellable :
provider.defaults.cancellable;
}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 25, 2015

Author Member

@petebacondarwin, this final implementation is little bit different that the one in the PR:

If timeout is non-numeric, then cancellable will be false. In the PR, it would be calculated as if timeout was not specified. Was it an intentional change ?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 25, 2015

Contributor

So you are saying that we should accept a timeout that is non-numeric alongside cancellable: true? That seems a strange mix of deprecated and new functionality...

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 25, 2015

Author Member

It's a matter of taste/implementation detail. I just wanted to make sure it was an intentional change.

BTW, the reasoning behind calculating cancellable even if timeout was a promise, is that cancellable could also be configured "resource-class-wide" (using the options parameter) or globally (using $resourceProvider.defaults), so trying to calculate cancellable whenever possible was an attempt to stay closer to the developer's intent.

Would be fine either way (as long as it is a conscious choice 😃)


Resource[name] = function(a1, a2, a3, a4) {
var params = {}, data, success, error;
Expand Down Expand Up @@ -572,6 +640,7 @@ angular.module('ngResource', ['ng']).
defaultResponseInterceptor;
var responseErrorInterceptor = action.interceptor && action.interceptor.responseError ||
undefined;
var timeoutDeferred;

forEach(action, function(value, key) {
switch (key) {
Expand All @@ -581,21 +650,23 @@ angular.module('ngResource', ['ng']).
case 'params':
case 'isArray':
case 'interceptor':
break;
case 'timeout':
httpConfig[key] = value;
case 'cancellable':
break;
}
});

if (!isInstanceCall && cancellable) {
timeoutDeferred = $q.defer();
httpConfig.timeout = timeoutDeferred.promise;
}

if (hasBody) httpConfig.data = data;
route.setUrlParams(httpConfig,
extend({}, extractParams(data, action.params || {}), params),
action.url);

var promise = $http(httpConfig).then(function(response) {
var data = response.data,
promise = value.$promise;
var data = response.data;

if (data) {
// Need to convert action.isArray to boolean in case it is undefined
Expand All @@ -620,24 +691,27 @@ angular.module('ngResource', ['ng']).
}
});
} else {
var promise = value.$promise; // Save the promise
shallowClearAndCopy(data, value);
value.$promise = promise;
value.$promise = promise; // Restore the promise
}
}

value.$resolved = true;

response.resource = value;

return response;
}, function(response) {
value.$resolved = true;

(error || noop)(response);

return $q.reject(response);
});

promise.finally(function() {
value.$resolved = true;
if (cancellable) {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 25, 2015

Author Member

@petebacondarwin, won't this add the method even to non-instance call return values ?
Was this change intentional ?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 25, 2015

Contributor

Right! Sorry. It should be if (!isInstanceCall && cancellable) {

value.$cancelRequest = angular.noop;
timeoutDeferred = httpConfig.timeout = null;
}
});

promise = promise.then(
function(response) {
var value = responseInterceptor(response);
Expand All @@ -652,6 +726,7 @@ angular.module('ngResource', ['ng']).
// - return the instance / collection
value.$promise = promise;
value.$resolved = false;
if (cancellable) value.$cancelRequest = timeoutDeferred.resolve;

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 25, 2015

Author Member

@petebacondarwin, won't this add the method even to non-instance call return values ?
Was this change intentional ?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 25, 2015

Contributor

Same as above sorry

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 25, 2015

Contributor

Actually on line 723 we are already testing for instance call.


return value;
}
Expand Down
Loading

8 comments on commit 98528be

@mrac
Copy link
Contributor

@mrac mrac commented on 98528be Jan 21, 2016

Choose a reason for hiding this comment

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

Why it's not allowed to cancel a request that has a numeric timeout?

@gkalpak
Copy link
Member Author

Choose a reason for hiding this comment

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

@mrac, both mechansms (numeric timeout and cancellable) use the same $http API (the timeout property of the $http config object). So, there can only be one in effect per request.

We needed to pick one to take precedence over the other, so we picked the numeric timeout.

@mrac
Copy link
Contributor

@mrac mrac commented on 98528be Jan 21, 2016

Choose a reason for hiding this comment

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

@gkalpak Thanks, then I'll look for a way to simulate timeout by using $timeout with $cancelRequest.

@gkalpak
Copy link
Member Author

Choose a reason for hiding this comment

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

@mrac, yes, that's certainly doable. In fact, thinking about it again, we should be able to do the same thing in $resource. I.e. when something is cancellable and has a numeric timeout, use $timeout to resolve the promise.

Would you like to try your luck on a PR for that ?

@mrac
Copy link
Contributor

@mrac mrac commented on 98528be Jan 21, 2016

Choose a reason for hiding this comment

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

@gkalpak sure, why not

@gkalpak
Copy link
Member Author

Choose a reason for hiding this comment

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

@mrac, awesome ! Feel free to ping me if you need any help and also when you submit the PR, so I can take a look.

@mrac
Copy link
Contributor

@mrac mrac commented on 98528be Jan 22, 2016

Choose a reason for hiding this comment

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

@gkalpak here you go:
feat($resource): add support for timeout in cancellable actions #13824

@gkalpak
Copy link
Member Author

Choose a reason for hiding this comment

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

That was quick 😃
Will take a look asap. Thx !

Please sign in to comment.