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

feat($resource): add support for timeout in cancellable actions #13824

Closed
wants to merge 2 commits into from
Closed

feat($resource): add support for timeout in cancellable actions #13824

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be56c9e
feat($resource): add support for timeout in cancellable actions
mrac Jan 22, 2016
c1c6e9c
fixup: simplify $timeout handler
mrac Jan 25, 2016
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
28 changes: 22 additions & 6 deletions src/ngResource/resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ function shallowClearAndCopy(src, dst) {
* @requires $http
* @requires ng.$log
* @requires $q
* @requires $timeout
*
* @description
* A factory which creates a resource object that lets you interact with
Expand Down Expand Up @@ -160,7 +161,6 @@ function shallowClearAndCopy(src, dst) {
* 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 Down Expand Up @@ -416,7 +416,7 @@ angular.module('ngResource', ['ng']).
}
};

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

var noop = angular.noop,
forEach = angular.forEach,
Expand Down Expand Up @@ -575,7 +575,11 @@ angular.module('ngResource', ['ng']).

forEach(actions, function(action, name) {
var hasBody = /^(POST|PUT|PATCH)$/i.test(action.method);
var cancellable = false;
var cancellable = angular.isDefined(action.cancellable) ? action.cancellable :
(options && angular.isDefined(options.cancellable)) ? options.cancellable :
provider.defaults.cancellable;

var simulatedTimeout;
Copy link
Member

Choose a reason for hiding this comment

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

simulatedTimeout is a little confusing imo (what is "simulated" about it).
(On the other hand, I'm notiriously bad with names 😃)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to find a different name, not to confuse with timeout which is passed to $http. There are quite a few variables that deal with $http timeout.

So for me it means "non-$http" timeout.


if (!angular.isNumber(action.timeout)) {
if (action.timeout) {
Expand All @@ -586,9 +590,11 @@ angular.module('ngResource', ['ng']).
'requests, you should use the `cancellable` option.');
delete action.timeout;
}
cancellable = angular.isDefined(action.cancellable) ? action.cancellable :
(options && angular.isDefined(options.cancellable)) ? options.cancellable :
provider.defaults.cancellable;
} else {
if(cancellable) {
simulatedTimeout = action.timeout;
delete action.timeout;
Copy link
Member

Choose a reason for hiding this comment

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

Why delete it here ? Couldn't we just let it be ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we don't need to delete it. It gets overwritten anyway later:

httpConfig.timeout = timeoutDeferred.promise;

Copy link
Member

Choose a reason for hiding this comment

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

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 😃

}
}

Resource[name] = function(a1, a2, a3, a4) {
Expand Down Expand Up @@ -639,6 +645,7 @@ angular.module('ngResource', ['ng']).
var responseErrorInterceptor = action.interceptor && action.interceptor.responseError ||
undefined;
var timeoutDeferred;
var simulatedTimeoutPromise;

forEach(action, function(value, key) {
switch (key) {
Expand All @@ -656,6 +663,14 @@ angular.module('ngResource', ['ng']).
if (!isInstanceCall && cancellable) {
timeoutDeferred = $q.defer();
httpConfig.timeout = timeoutDeferred.promise;

if(simulatedTimeout) {
simulatedTimeoutPromise = $timeout(function() {
if(timeoutDeferred) {
timeoutDeferred.resolve();
}
}, simulatedTimeout);
Copy link
Member

Choose a reason for hiding this comment

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

This could be just simulatedTimeoutPromise = $timeout(timeoutDeferred.resolve, simulatedTimeout);.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I wanted to be sure, timeoutDeferred is not null already, so extra if() here. But may be it's obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, timeoutDeferred.resolve can be evaluated instantly (and not inside the timeout handler).

}
}

if (hasBody) httpConfig.data = data;
Expand Down Expand Up @@ -707,6 +722,7 @@ angular.module('ngResource', ['ng']).
if (!isInstanceCall && cancellable) {
value.$cancelRequest = angular.noop;
timeoutDeferred = httpConfig.timeout = null;
$timeout.cancel(simulatedTimeoutPromise);
Copy link
Member

Choose a reason for hiding this comment

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

Also set simulatedTimeoutPromise to null (just in case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

}
});

Expand Down
34 changes: 30 additions & 4 deletions test/ngResource/resourceSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1372,6 +1372,7 @@ describe('cancelling requests', function() {
var httpSpy;
var $httpBackend;
var $resource;
var $timeout;

beforeEach(module('ngResource', function($provide) {
$provide.decorator('$http', function($delegate) {
Expand All @@ -1380,9 +1381,10 @@ describe('cancelling requests', function() {
});
}));

beforeEach(inject(function(_$httpBackend_, _$resource_) {
beforeEach(inject(function(_$httpBackend_, _$resource_, _$timeout_) {
$httpBackend = _$httpBackend_;
$resource = _$resource_;
$timeout = _$timeout_;
}));

it('should accept numeric timeouts in actions and pass them to $http', function() {
Expand Down Expand Up @@ -1531,7 +1533,9 @@ describe('cancelling requests', function() {
expect(creditCard3.$cancelRequest).toBeUndefined();
});

it('should not make the request cancellable if there is a timeout', function() {
it('should accept numeric timeouts in cancellable actions and cancel the request when timeout occurs', function() {
$httpBackend.whenGET('/CreditCard').respond({});

var CreditCard = $resource('/CreditCard', {}, {
get: {
method: 'GET',
Expand All @@ -1540,9 +1544,13 @@ describe('cancelling requests', function() {
}
});

var creditCard = CreditCard.get();
CreditCard.get();
$timeout.flush();
expect($httpBackend.flush).toThrow(new Error('No pending request to flush !'));

CreditCard.get();
expect($httpBackend.flush).not.toThrow();

expect(creditCard.$cancelRequest).toBeUndefined();
});

it('should cancel the request (if cancellable), when calling `$cancelRequest`', function() {
Expand All @@ -1562,6 +1570,24 @@ describe('cancelling requests', function() {
expect($httpBackend.flush).not.toThrow();
});

it('should cancel the request, when calling `$cancelRequest` in cancellable actions with timeout defined', function() {
$httpBackend.whenGET('/CreditCard').respond({});

var CreditCard = $resource('/CreditCard', {}, {
get: {
method: 'GET',
timeout: 10000,
cancellable: true
}
});

CreditCard.get().$cancelRequest();
expect($httpBackend.flush).toThrow(new Error('No pending request to flush !'));

CreditCard.get();
expect($httpBackend.flush).not.toThrow();
});

it('should reset `$cancelRequest` after the response arrives', function() {
$httpBackend.whenGET('/CreditCard').respond({});

Expand Down