Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alternative $resource cancellable API #4

Conversation

petebacondarwin
Copy link

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.
  • Provide a cancelRequest static method on the Resource that will abort
    the request (if it's not already completed or aborted).
    If there is a numeric timeout specified on the action's configuration,
    this method will have no effect.

Example usage:

var Post = $resource('/posts/:id', {id: '@id'}, {
  get: {
    method: 'GET'
  }
});

var currentPost = Post.get({id: 1});
...
// A moment later the user selects another post, so
// we don't need the previous request any more
Post.cancelRequest(currentPost);
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:

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:

var User = $resource('/api/user/:id', {id: '@id'}, {
  get: {method: 'GET'}
});

var user = User.get({id: 1});   // sends a request
User.cancelRequest(instance);   // aborts the request

user = User.get({id: 2});

Fixes angular#9332
Closes angular#13050
Closes angular#13058

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.
- Provide a `cancelRequest` static method on the Resource that will abort
  the request (if it's not already completed or aborted).
  If there is a numeric `timeout` specified on the action's configuration,
  this method will have no effect.

Example usage:

```js
var Post = $resource('/posts/:id', {id: '@id'}, {
  get: {
    method: 'GET'
  }
});

var currentPost = Post.get({id: 1});
...
// A moment later the user selects another post, so
// we don't need the previous request any more
Post.cancelRequest(currentPost);
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'}
});

var user = User.get({id: 1});   // sends a request
User.cancelRequest(instance);   // aborts the request

user = User.get({id: 2});
```

Fixes angular#9332
Closes angular#13050
Closes angular#13058
@petebacondarwin
Copy link
Author

The reason for the massive changes to the spec is that I wrapped all three describe blocks in a single block and indented it all. If you do an ignore-whitespace diff then you can see what changed easily.

@gkalpak
Copy link
Owner

gkalpak commented Nov 24, 2015

A few thoughts:

  • This PR removes the cancellable feature, which controls whether or not a promise needs to be created. Perf implications shouldn't be noticeable, but I would still avoid creating/resolving promises unnecessarily.
  • Again the perf hit shouldn't be noticeable, but I would still avoid having to do 2-3 HashMap operations per call, if possible.
  • From a usability point of view, having the method is a little more explicit, while passing the wrong object to Resource.cancelRequest() will just fail silently.

These are pretty minor comments, so I am totally OK with both approaches.
I slightly favor the original one, because (besides the above thoughts) it's more consistent with how things are done to far (e.g. see $promise, $resolved) and the internal implmentation isn't more complex (if we ignore cancellable which is independent of either approach).

tl;dr
Both approaches work fine for me (and are much better than what we have right now).

@gkalpak gkalpak closed this Nov 26, 2015
@petebacondarwin petebacondarwin deleted the pr-13210 branch November 24, 2016 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants