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

Fix confusing behaviour of ngResource callbacks #6865

Closed
wants to merge 2 commits into from
Closed

Fix confusing behaviour of ngResource callbacks #6865

wants to merge 2 commits into from

Conversation

alexsanford
Copy link
Contributor

Request Type: bug

How to reproduce: Create a resource and add a response interceptor to an action. Make the interceptor return a function. Call the action and check the parameter handed into the callback function. The parameter will be a promise, not the resolved value of that promise.

Component(s): ngResource

Impact: small

Complexity: small

This issue is related to:

Detailed Description:

When using an interceptor in ngResource, if the interceptor returns a promise, this promise is given to the success callback. Instead, the object which resolves the promise should be given to the success callback.

Other Comments:

This fixes issue #6731

@alexsanford
fix(ngResource): do not call callback with a promise
8ee0864 
Call the success and failure callbacks for an action with the response
object. Never call it with a promise object.

Closes angular#6731
@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#6865)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@g00fy-
Copy link

g00fy- commented Mar 27, 2014

+1, not sure why travis failed ;/.

@alexsanford
Copy link
Contributor Author

Hmmm...I'm seeing another couple of problems. This commit solves the issue when the response interceptor returns a promise which is then resolved. However:

  1. If the promise returned by the response interceptor is rejected instead of resolved, no callback is called. Instead, I think that the error callback should be called with the promise's rejection value in this case.
  2. The error callback is always called with the original $http response, regardless of the return value of the responseError interceptor. However, I would say that it should instead be called with the return value, since that's the point of having an interceptor. Furthermore, if the responseError interceptor returns a promise, then the success or error callback should be called with the resolution or rejection value of the promise, respectively.

I believe that this would make the callbacks behave less unexpectedly, and better leverage the underlying promise architecture.

Any thoughts?

@alexsanford
fix(ngResource): make success and error callbacks behave more expectedly
ecdaf66 
Call the success and error callbacks with the values returned by the
response and responseError interceptors, respectively. If the
interceptor returns a promise, call the appropriate callback with the
resolution or rejection value of that promise. Previously, the error
callback was called with the server response before calling the
interceptor, regardless of whether the error interceptor handled the
error, and regardless of what it returned. Also, if the server response
was successful, but the response interceptor returned a rejection, no
callback was called.
@alexsanford
Copy link
Contributor Author

Added a commit that provides the functionality described in my last post.

@RomainLK
Copy link

+1; found those issues while unit testing a wrapper around $resource. At the moment, interceptors behave quite erratically depending on whether we are using a callback or a promise. The functions you described would fix this.

@Narretz Narretz added this to the Backlog milestone Jul 2, 2014
@btford btford removed the gh: PR label Aug 20, 2014
@jeffbcross jeffbcross force-pushed the master branch 2 times, most recently from cad9560 to f294244 Compare October 2, 2014 22:09
@Narretz Narretz mentioned this pull request Feb 16, 2018
Closed
gkalpak added a commit to gkalpak/angular.js that referenced this pull request Feb 16, 2018
@gkalpak
fix($resource): fix interceptors and success/error callbacks
743c4e4 
Previously, action-specific interceptors and `success`/`error` callbacks
were executed in inconsistent relative orders and in a way that did not
meet the general expectation for interceptor behavior (e.g. ability to
recover from errors, performing asynchronous operations, etc).

This commit fixes the behavior to make it more consistent and expected.
The main differences are that `success`/`error` callbacks will now be
run _after_ `response`/`responseError` interceptors complete (even if
interceptors return a promise) and the correct callback will be called
based on the result of the interceptor (e.g. if the `responseError`
interceptor recovers from an error, the `success` callback will be
called).
See also angular#9334 (comment).

This commit also replaces the use of `success`/`error` callbacks in the
docs with using the returned promise.

Fixes angular#6731
Fixes angular#9334
Closes angular#6865

BREAKING CHANGE:

If you are not using `success` or `error` callbacks with `$resource`,
your app should not be affected by this change.

If you are using `success` or `error` callbacks (with or without
response interceptors), one (subtle) difference is that throwing an
error inside the callbacks will not propagate to the returned
`$promise`. Therefore, you should try to use the promises whenever
possible. E.g.:

```js
// Avoid
User.query(function onSuccess(users) { throw new Error(); }).
  $promise.
  catch(function onError() { /* Will not be called. */ });

// Prefer
User.query().
  $promise.
  then(function onSuccess(users) { throw new Error(); }).
  catch(function onError() { /* Will be called. */ });
```

Finally, if you are using `success` or `error` callbacks with response
interceptors, the callbacks will now always run _after_ the interceptors
(and wait for them to resolve in case they return a promise).
Previously, the `error` callback was called before the `responseError`
interceptor and the `success` callback was synchronously called after
the `response` interceptor. E.g.:

```js
var User = $resource('/api/users/:id', {id: '@id'}, {
  get: {
    method: 'get',
    interceptor: {
      response: function(response) {
        console.log('responseInterceptor-1');
        return $timeout(1000).then(function() {
          console.log('responseInterceptor-2');
          return response.resource;
        });
      },
      responseError: function(response) {
        console.log('responseErrorInterceptor-1');
        return $timeout(1000).then(function() {
          console.log('responseErrorInterceptor-2');
          return $q.reject('Ooops!');
        });
      }
    }
  }
});
var onSuccess = function(value) { console.log('successCallback', value); };
var onError = function(error) { console.log('errorCallback', error); };

// Assuming the following call is successful...
User.get({id: 1}, onSuccess, onError);
  // Old behavior:
  //   responseInterceptor-1
  //   successCallback, {/* Promise object */}
  //   responseInterceptor-2
  // New behavior:
  //   responseInterceptor-1
  //   responseInterceptor-2
  //   successCallback, {/* User object */}

// Assuming the following call returns an error...
User.get({id: 2}, onSuccess, onError);
  // Old behavior:
  //   errorCallback, {/* Response object */}
  //   responseErrorInterceptor-1
  //   responseErrorInterceptor-2
  // New behavior:
  //   responseErrorInterceptor-1
  //   responseErrorInterceptor-2
  //   errorCallback, Ooops!
```
gkalpak added a commit to gkalpak/angular.js that referenced this pull request Feb 20, 2018
@gkalpak
fix($resource): fix interceptors and success/error callbacks
21ee096 
Previously, action-specific interceptors and `success`/`error` callbacks
were executed in inconsistent relative orders and in a way that did not
meet the general expectation for interceptor behavior (e.g. ability to
recover from errors, performing asynchronous operations, etc).

This commit fixes the behavior to make it more consistent and expected.
The main differences are that `success`/`error` callbacks will now be
run _after_ `response`/`responseError` interceptors complete (even if
interceptors return a promise) and the correct callback will be called
based on the result of the interceptor (e.g. if the `responseError`
interceptor recovers from an error, the `success` callback will be
called).
See also angular#9334 (comment).

This commit also replaces the use of `success`/`error` callbacks in the
docs with using the returned promise.

Fixes angular#6731
Fixes angular#9334
Closes angular#6865

BREAKING CHANGE:

If you are not using `success` or `error` callbacks with `$resource`,
your app should not be affected by this change.

If you are using `success` or `error` callbacks (with or without
response interceptors), one (subtle) difference is that throwing an
error inside the callbacks will not propagate to the returned
`$promise`. Therefore, you should try to use the promises whenever
possible. E.g.:

```js
// Avoid
User.query(function onSuccess(users) { throw new Error(); }).
  $promise.
  catch(function onError() { /* Will not be called. */ });

// Prefer
User.query().
  $promise.
  then(function onSuccess(users) { throw new Error(); }).
  catch(function onError() { /* Will be called. */ });
```

Finally, if you are using `success` or `error` callbacks with response
interceptors, the callbacks will now always run _after_ the interceptors
(and wait for them to resolve in case they return a promise).
Previously, the `error` callback was called before the `responseError`
interceptor and the `success` callback was synchronously called after
the `response` interceptor. E.g.:

```js
var User = $resource('/api/users/:id', {id: '@id'}, {
  get: {
    method: 'get',
    interceptor: {
      response: function(response) {
        console.log('responseInterceptor-1');
        return $timeout(1000).then(function() {
          console.log('responseInterceptor-2');
          return response.resource;
        });
      },
      responseError: function(response) {
        console.log('responseErrorInterceptor-1');
        return $timeout(1000).then(function() {
          console.log('responseErrorInterceptor-2');
          return $q.reject('Ooops!');
        });
      }
    }
  }
});
var onSuccess = function(value) { console.log('successCallback', value); };
var onError = function(error) { console.log('errorCallback', error); };

// Assuming the following call is successful...
User.get({id: 1}, onSuccess, onError);
  // Old behavior:
  //   responseInterceptor-1
  //   successCallback, {/* Promise object */}
  //   responseInterceptor-2
  // New behavior:
  //   responseInterceptor-1
  //   responseInterceptor-2
  //   successCallback, {/* User object */}

// Assuming the following call returns an error...
User.get({id: 2}, onSuccess, onError);
  // Old behavior:
  //   errorCallback, {/* Response object */}
  //   responseErrorInterceptor-1
  //   responseErrorInterceptor-2
  // New behavior:
  //   responseErrorInterceptor-1
  //   responseErrorInterceptor-2
  //   errorCallback, Ooops!
```
gkalpak added a commit to gkalpak/angular.js that referenced this pull request Feb 20, 2018
@gkalpak
fix($resource): fix interceptors and success/error callbacks
d9dc7b9 
Previously, action-specific interceptors and `success`/`error` callbacks
were executed in inconsistent relative orders and in a way that did not
meet the general expectation for interceptor behavior (e.g. ability to
recover from errors, performing asynchronous operations, etc).

This commit fixes the behavior to make it more consistent and expected.
The main differences are that `success`/`error` callbacks will now be
run _after_ `response`/`responseError` interceptors complete (even if
interceptors return a promise) and the correct callback will be called
based on the result of the interceptor (e.g. if the `responseError`
interceptor recovers from an error, the `success` callback will be
called).
See also angular#9334 (comment).

This commit also replaces the use of `success`/`error` callbacks in the
docs with using the returned promise.

Fixes angular#6731
Fixes angular#9334
Closes angular#6865

BREAKING CHANGE:

If you are not using `success` or `error` callbacks with `$resource`,
your app should not be affected by this change.

If you are using `success` or `error` callbacks (with or without
response interceptors), one (subtle) difference is that throwing an
error inside the callbacks will not propagate to the returned
`$promise`. Therefore, you should try to use the promises whenever
possible. E.g.:

```js
// Avoid
User.query(function onSuccess(users) { throw new Error(); }).
  $promise.
  catch(function onError() { /* Will not be called. */ });

// Prefer
User.query().
  $promise.
  then(function onSuccess(users) { throw new Error(); }).
  catch(function onError() { /* Will be called. */ });
```

Finally, if you are using `success` or `error` callbacks with response
interceptors, the callbacks will now always run _after_ the interceptors
(and wait for them to resolve in case they return a promise).
Previously, the `error` callback was called before the `responseError`
interceptor and the `success` callback was synchronously called after
the `response` interceptor. E.g.:

```js
var User = $resource('/api/users/:id', {id: '@id'}, {
  get: {
    method: 'get',
    interceptor: {
      response: function(response) {
        console.log('responseInterceptor-1');
        return $timeout(1000).then(function() {
          console.log('responseInterceptor-2');
          return response.resource;
        });
      },
      responseError: function(response) {
        console.log('responseErrorInterceptor-1');
        return $timeout(1000).then(function() {
          console.log('responseErrorInterceptor-2');
          return $q.reject('Ooops!');
        });
      }
    }
  }
});
var onSuccess = function(value) { console.log('successCallback', value); };
var onError = function(error) { console.log('errorCallback', error); };

// Assuming the following call is successful...
User.get({id: 1}, onSuccess, onError);
  // Old behavior:
  //   responseInterceptor-1
  //   successCallback, {/* Promise object */}
  //   responseInterceptor-2
  // New behavior:
  //   responseInterceptor-1
  //   responseInterceptor-2
  //   successCallback, {/* User object */}

// Assuming the following call returns an error...
User.get({id: 2}, onSuccess, onError);
  // Old behavior:
  //   errorCallback, {/* Response object */}
  //   responseErrorInterceptor-1
  //   responseErrorInterceptor-2
  // New behavior:
  //   responseErrorInterceptor-1
  //   responseErrorInterceptor-2
  //   errorCallback, Ooops!
```
@gkalpak gkalpak closed this in ea05857 Feb 20, 2018
Narretz pushed a commit to Narretz/angular.js that referenced this pull request Feb 23, 2018
@gkalpak @Narretz
fix($resource): fix interceptors and success/error callbacks
fdaf792 
Previously, action-specific interceptors and `success`/`error` callbacks
were executed in inconsistent relative orders and in a way that did not
meet the general expectation for interceptor behavior (e.g. ability to
recover from errors, performing asynchronous operations, etc).

This commit fixes the behavior to make it more consistent and expected.
The main differences are that `success`/`error` callbacks will now be
run _after_ `response`/`responseError` interceptors complete (even if
interceptors return a promise) and the correct callback will be called
based on the result of the interceptor (e.g. if the `responseError`
interceptor recovers from an error, the `success` callback will be
called).
See also angular#9334 (comment).

This commit also replaces the use of `success`/`error` callbacks in the
docs with using the returned promise.

Fixes angular#6731
Fixes angular#9334
Closes angular#6865

BREAKING CHANGE:

If you are not using `success` or `error` callbacks with `$resource`,
your app should not be affected by this change.

If you are using `success` or `error` callbacks (with or without
response interceptors), one (subtle) difference is that throwing an
error inside the callbacks will not propagate to the returned
`$promise`. Therefore, you should try to use the promises whenever
possible. E.g.:

```js
// Avoid
User.query(function onSuccess(users) { throw new Error(); }).
  $promise.
  catch(function onError() { /* Will not be called. */ });

// Prefer
User.query().
  $promise.
  then(function onSuccess(users) { throw new Error(); }).
  catch(function onError() { /* Will be called. */ });
```

Finally, if you are using `success` or `error` callbacks with response
interceptors, the callbacks will now always run _after_ the interceptors
(and wait for them to resolve in case they return a promise).
Previously, the `error` callback was called before the `responseError`
interceptor and the `success` callback was synchronously called after
the `response` interceptor. E.g.:

```js
var User = $resource('/api/users/:id', {id: '@id'}, {
  get: {
    method: 'get',
    interceptor: {
      response: function(response) {
        console.log('responseInterceptor-1');
        return $timeout(1000).then(function() {
          console.log('responseInterceptor-2');
          return response.resource;
        });
      },
      responseError: function(response) {
        console.log('responseErrorInterceptor-1');
        return $timeout(1000).then(function() {
          console.log('responseErrorInterceptor-2');
          return $q.reject('Ooops!');
        });
      }
    }
  }
});
var onSuccess = function(value) { console.log('successCallback', value); };
var onError = function(error) { console.log('errorCallback', error); };

// Assuming the following call is successful...
User.get({id: 1}, onSuccess, onError);
  // Old behavior:
  //   responseInterceptor-1
  //   successCallback, {/* Promise object */}
  //   responseInterceptor-2
  // New behavior:
  //   responseInterceptor-1
  //   responseInterceptor-2
  //   successCallback, {/* User object */}

// Assuming the following call returns an error...
User.get({id: 2}, onSuccess, onError);
  // Old behavior:
  //   errorCallback, {/* Response object */}
  //   responseErrorInterceptor-1
  //   responseErrorInterceptor-2
  // New behavior:
  //   responseErrorInterceptor-1
  //   responseErrorInterceptor-2
  //   errorCallback, Ooops!
```
@dependencies dependencies bot mentioned this pull request May 11, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes component: ngResource frequency: low severity: inconvenient
Projects
None yet
Milestone
Backlog - won't fix
Development

Successfully merging this pull request may close these issues.
7 participants
@alexsanford @mary-poppins @g00fy- @RomainLK @linclark @btford @Narretz