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

Conversation

arty-name
Copy link
Contributor

$resource "class" actions delegate to $http in a fashion that does not allow to pass timeout promise to individual requests. Hence these individual requests can not be cancelled. This patch adds $abort method to the object returned by $resource "class" actions.

Closes #5612

@IgorMinar
Copy link
Contributor

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

Artemy Tregubenko and others added 2 commits January 3, 2014 14:14
…"class" actions

`$resource` "class" actions delegate to `$http` in a fashion that does not allow to pass timeout promise to individual requests. Hence these individual requests can not be cancelled. This patch adds $abort method to the object returned by `$resource` "class" actions.

Closes angular#5612
@arty-name
Copy link
Contributor Author

Hi Igor. I've signed Google CLA on 2008-02-29 as Artemy Tregubenko or Artemy Tregoubenko. I suppose the email was atregoubenko@gmail.com, but it could have been me@arty.name as well. I've updated the commit author to be atregoubenko@gmail.com. Should I change it to me@arty.name instead?

@ghost ghost assigned tbosch Jan 4, 2014
@IgorMinar
Copy link
Contributor

please sign the CLA again with the new email address.

however to be honest given the history of aborting/canceling anything promise related, I'm not too hopeful about this PR.

additionally as it stands right now the abort method is associated with the resource instance and not with a request generated by that resource instance, so it's not quite clear which request will be aborted if there are multiple requests in flight.

have you thought about these things?

@arty-name
Copy link
Contributor Author

I have signed the CLA for atregoubenko@gmail.com

I am not too optimistic about the future of this proposal either, as I have seen the discussions about cancelling, and the current approach of returning promises doesn't suit cancelling very well. However I think this feature is important and I want to try to push for it, even if that means slight architecture change.

I disagree with you that currently the abort method is associated with the resource instance. The deferred that provides the promise for the timeout is scoped to a particular call to a resource action, and is passed to one $http call only, so no matter the amount of parallel requests, only one of them will be aborted. Here's an example:

var resource = $resource(…);
resource.$abort // undefined
var request1 = resource.query(…); // creates deferred1
var request2 = resource.query(…); // creates deferred2
request1.$abort(); // will resolve deferred1 only, but not deferred2, so request2 will continue

@michalgm
Copy link

I just want to say thanks for writing this! I spent a long time searching for some way abort specific $resource requests. One question though - is there a way to differentiate in the error handling callback between a request that timed out and a request that was manually aborted?
Thanks again!

@arty-name
Copy link
Contributor Author

Hi Greg, potentially the promise used to abort or timeout the request may be resolved with different values in each case, and your error callback can check it.

@rainboxx
Copy link

rainboxx commented Mar 4, 2014

@IgorMinar can you take another look at this PR and @arty-name's explanations?

In a current project we have the $resource declaration in a separate factory so we can use it from multiple points in the code. This, however, prevents us to use a promise as the value for timeout, since once resolved we'd had to reset it inconveniently.

The PR seems to be a much better approach.

Thanks.

// 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.

@IgorMinar IgorMinar modified the milestones: Purgatory, Backlog Apr 1, 2014
@ghost
Copy link

ghost commented Jun 30, 2014

IDK if this is the best implementation or not, but regardless we need to be able to abort individual resource requests. A guy on the Rackspace blog developed some methods to make it possible and simple, but it seems wrong that he had to do all this to get this basic functionality. http://developer.rackspace.com/blog/cancelling-ajax-requests-in-angularjs-applications.html

I need to abort requests if people click too fast on a pager. There are probably countless other times people will want this feature.

@rpocklin
Copy link

After much reading into the historical issues and PRs surrounding this topic, i've come to the following conclusion:

  1. The ideal solution is non-trivial and still up for debate.
  2. ng-resource abstracts $http requests as promises.
  3. Promise don't currently support the notion of cancellation, only resolve and reject.

Until an ultimate solution can be resolved we need access to the underlying xhr or $http request. Alternatively just update the documentation to admit this limitation since many people have assumed this feature would just be available.

FWIW i'm defining custom methods on my model object which delegates to $http, which can take the timeout promise directly. I only needed this feature since I am dealing with polling of real-time data but there are many possible use cases.

@arty-name
Copy link
Contributor Author

Hi. I am closing this PR to explicitly show it's not maintained anymore. Everyone interested are welcome to fork the branch to continue maintenance. Our team has switched to using $http since. I agree with @rpocklin that exposing XHR object for now might be a more flexible solution. I suppose this discussion demonstrates downsides of representing a request object by promise.

@arty-name arty-name closed this Aug 31, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Requests by $resource "class" actions can not be aborted
7 participants