feat($resource): add support for timeout in cancellable actions #13824
Conversation
Old behavior: actions can be either cancellable or have a numeric timeout.
When having both defined, cancellable was ignored.
With this commit: it's possible for actions to have both cancellable:true
and numeric timeout defined.
Example usage:
```js
var Post = $resource('/posts/:id', {id: '@id'}, {
get: {
method: 'GET',
cancellable: true,
timeout: 10000
}
});
var currentPost = Post.get({id: 1});
...
// the next request can cancel the previous one
currentPost.$cancelRequest();
currentPost = Post.get({id: 2});
// any of those requests will also timeout, if the response
// doesn't come within 10 seconds
```
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
| } else { | ||
| if(cancellable) { | ||
| simulatedTimeout = action.timeout; | ||
| delete action.timeout; |
There was a problem hiding this comment.
Why delete it here ? Couldn't we just let it be ?
There was a problem hiding this comment.
Yes we don't need to delete it. It gets overwritten anyway later:
httpConfig.timeout = timeoutDeferred.promise;
There was a problem hiding this comment.
I don't think it is overwritten. action.timeout is not the same as httpConfig.timeout.
But we shouldn't delete it if we don't have to 😃
|
A couple of minor comments, but LGTM otherwise. |
simplify $timeout handler set `simulatedTimeoutPromise` to null don't delete action.timeout - not necessary
|
@mrac, thx for working on this ✨ 👍 ✨ BTW (so you know for the next time), there were some legitimate failures in tests - nothing major just a couple of whitespace issues that JSCS (rightfully) complaints about 😃 |
|
@gkalpak great, thanks! Good to hear it's going to be in 1.5.0. |
|
@gkalpak It's nice to have it released already, thanks! Still here remains a problem of how to distinguish between cancelled requests and timed-out requests. The latter might need different handling in most cases. Apparently both produce the same error response The solution would be to allow to pass a statusConfig to timeout-promise resolve value in $http. It could produce different error status and statusText. Currently timeout-promise resolve value is ignored entirely. |
|
I don't think it's high priority, but I would be happy to review any PRs if anyone decided to submit them 😉 |
Old behavior: actions can be either cancellable or have a numeric timeout.
When having both defined, cancellable was ignored.
With this commit: it's possible for actions to have both cancellable:true
and numeric timeout defined.
Example usage: