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

Add abort method to $http promise. #1623

Closed
wants to merge 4 commits into from
Closed

Conversation

dbinit
Copy link
Contributor

@dbinit dbinit commented Nov 29, 2012

This is an attempt at resolving issue #1159. Let me know what you think.

@mhevery
Copy link
Contributor

mhevery commented Jan 17, 2013

Hi DbInit,

Thanks for this pull request. We are very interested in having a solution to this problem, but there are few issues with this proposal, so we can't accept it in this form. I am hoping that you could address these so that we can get this in.

  1. The contract of .abort() should be that after calling it, the promise is guaranteed to be resolved with a rejection. (unless it has already been resolved in which cas it is a noop.)
  2. Calling .abort() should return a boolean
    • True - if successfully aborted
    • False - if abort failed because the promise has already been resolved.

In the current implementation this is not the cases:

If the request is cached it will resolve immediately https://github.com/angular/angular.js/blob/master/src/ng/http.js#L731 But the way promises resolve the .then only gets called on next $digest() see the nextTick implementation here: https://github.com/angular/angular.js/blob/master/src/ng/q.js#L155

The following behavior will be a surprise to the developer and is incorrect:

cache = $cacheFactory();
cache.put('/alreadyCachedURL', 'content');
promise = $http.get('/alreadyCachedURL', {cache:cache}).then(successSpy, rejectSpy);
promise.abort();
$rootScope.$digest();
expect(successSpy).not.toBeCalled(); // this will fail as it was called
expect(rejectSpy).toBeCalled(); // this will fail as it was not called.

Another issue is that we need httpBackend mock implementation to support aborting as well. See https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L817

I am going to close this PR to keep our PR queue clean. Please reopen it once you have fixed this use case.

@mhevery mhevery closed this Jan 17, 2013
@dbinit
Copy link
Contributor Author

dbinit commented Jan 17, 2013

Thanks Miško,

Was the general approach acceptable (adding the abort method to the promise)?

Also, right now I've based this on the 1.0.x branch, but I realize that's probably reserved for bug fixes. Should I rebase this to master?

  • david

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants