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

feat(ngResource): Add $abort method to objects returned by $resource "class" actions #5613

Closed
wants to merge 3 commits into from
+47 −1
Closed

feat(ngResource): Add $abort method to objects returned by $resource "class" actions #5613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37d1e5e
feat(ngResource): Add $abort method to objects returned by $resource …
Jan 3, 2014
b8472b5
feat(ngResource): Add $abort method to objects returned by $resource …
Jan 3, 2014
117bca0
updated commit author to atregoubenko@gmail.com
Jan 3, 2014
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
16 changes: 15 additions & 1 deletion src/ngResource/resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,8 @@ function shallowClearAndCopy(src, dst) {
* rejection), `false` before that. Knowing if the Resource has been resolved is useful in
* data-binding.
*
* - `$abort`: a method to abort the request if it has not completed yet.
*
* @example
*
* # Credit card resource
Expand Down Expand Up @@ -303,7 +305,7 @@ function shallowClearAndCopy(src, dst) {
* </pre>
*/
angular.module('ngResource', ['ng']).
factory('$resource', ['$http', '$q', function($http, $q) {
factory('$resource', ['$http', '$q', '$timeout', function($http, $q, $timeout) {

var DEFAULT_ACTIONS = {
'get': {method:'GET'},
Expand Down Expand Up @@ -501,6 +503,17 @@ angular.module('ngResource', ['ng']).
}
});

// If parameters do not contain `timeout` which is a promise, create it,
// so that later this call can be aborted by resolving this promise.
var timeout = httpConfig.timeout;
var timeoutDeferred;
if (!timeout || !timeout.then) {
timeoutDeferred = $q.defer();
httpConfig.timeout = timeoutDeferred.promise;
// If timeout is specified in milliseconds, use it to abort via promise
if (timeout) $timeout(timeoutDeferred.resolve, timeout);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

there is no need for this hunk. if timeout property is set on the httpConfig object, this object is going to be passed into $http which will do exactly what you did here.

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 disagree. There are three options: no property, numeric value or promise value. Latter case I ignore. First case obviously needs handling. However even when numeric value comes in, I cannot just pass it further, as in this case I will not be able to cancel the request. So I have this small code duplication here, but it gives me access to deferred, and it gives me means to cancel the request.

if (hasBody) httpConfig.data = data;
route.setUrlParams(httpConfig,
extend({}, extractParams(data, action.params || {}), params),
Expand Down Expand Up @@ -557,6 +570,7 @@ angular.module('ngResource', ['ng']).
// - return the instance / collection
value.$promise = promise;
value.$resolved = false;
if (timeoutDeferred) value.$abort = timeoutDeferred.resolve;
Copy link
Contributor

Choose a reason for hiding this comment

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

why should this be done only for GETs?

I'm not a big fan of $abort method as it reverses the execution flow. This is why we implemented timeout in $http instead of cancel: #2529

Copy link
Contributor

Choose a reason for hiding this comment

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

and to be more precise, it's not just me. there is no consensus about how to handle cancelation in promise-based apis. currently there is no secure/reliable way to do it without compromising guarantees that the promise api already offers.

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 get the point about no consensus. I'm not saying this is The Solution. I just want it to be here in case you guys decide this one is the best. And I do think we need a way to prevent success callbacks from happening when user changed his mind while the request was in progress. Regarding GETs it's not intentional. I don't know the codebase well enough and I thought I've covered all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also it looks like there's a passing test for POST right in the next file of this PR.


return value;
}
Expand Down
32 changes: 32 additions & 0 deletions test/ngResource/resourceSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,22 @@ describe("resource", function() {
});


it('should add $abort method to the result object', function(){
$httpBackend.expect('POST', '/CreditCard/123!charge?amount=10', '{"auth":"abc"}').respond({success: 'ok'});
var errorCallback = jasmine.createSpy();
var cc = CreditCard.charge({id:123, amount:10}, {auth:'abc'});
cc.$promise.then(callback, errorCallback);

expect(cc.$abort).toEqual(jasmine.any(Function));
cc.$abort();

$httpBackend.verifyNoOutstandingExpectation();
$httpBackend.verifyNoOutstandingRequest();
expect(callback).not.toHaveBeenCalled();
expect(errorCallback).toHaveBeenCalled();
});


it('should return promise from action method calls', function() {
$httpBackend.expect('GET', '/CreditCard/123').respond({id: 123, number: '9876'});
var cc = new CreditCard({name: 'Mojo'});
Expand Down Expand Up @@ -837,6 +853,22 @@ describe("resource", function() {
expect(callback).toHaveBeenCalledOnce();
expect(ccs.$resolved).toBe(true);
});


it('should add $abort method to the result object', function(){
$httpBackend.expect('GET', '/CreditCard?key=value').respond([{id: 1}, {id: 2}]);
var errorCallback = jasmine.createSpy();
var ccs = CreditCard.query({key: 'value'});
ccs.$promise.then(callback, errorCallback);

expect(ccs.$abort).toEqual(jasmine.any(Function));
ccs.$abort();

$httpBackend.verifyNoOutstandingExpectation();
$httpBackend.verifyNoOutstandingRequest();
expect(callback).not.toHaveBeenCalled();
expect(errorCallback).toHaveBeenCalled();
});
});

it('should allow per action response interceptor that gets full response', function() {
Expand Down