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

ngResource interceptor ambiguity : callback invoked with promise or value #6731

Closed
g00fy- opened this issue Mar 18, 2014 · 2 comments
Closed

ngResource interceptor ambiguity : callback invoked with promise or value #6731

g00fy- opened this issue Mar 18, 2014 · 2 comments
Labels
component: ngResource frequency: low severity: confusing
Milestone
Backlog - won't fix

Comments

@g00fy-
Copy link

g00fy- commented Mar 18, 2014

Line https://github.com/angular/angular.js/blob/master/src/ngResource/resource.js#L562
invokes success callback with either explicit response object or promise.
This is inconsistent as sometimes

// posts may be array of ngResource or promise
Post.query(function(posts){ $scope.posts = posts })

and

// posts are always array of ngResource
$scope.posts  =Post.query()

IMHO it should be

$q.when(value).then(function(value){  
  (success||noop)(value, response.headers) 
});
@g00fy-
Copy link
Author

g00fy- commented Mar 18, 2014

According to https://github.com/angular/angular.js/blob/master/src/ng/http.js#L363
interceptor can return response directly or as a promise

@alexsanford
Copy link
Contributor

+1

alexsanford added a commit to alexsanford/angular.js that referenced this issue Mar 26, 2014
@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
@alexsanford alexsanford mentioned this issue Mar 26, 2014
Closed
alexsanford added a commit to alexsanford/angular.js that referenced this issue Mar 26, 2014
@alexsanford
fix(ngResource): do not call callback with a promise
07f4cb1 
Call the success and failure callbacks for an action with the response
object. Never call it with a promise object.

Closes angular#6731
alexsanford added a commit to alexsanford/angular.js that referenced this issue Mar 27, 2014
@alexsanford
fix(ngResource): do not call callback with a promise
c057087 
Call the success and failure callbacks for an action with the response
object. Never call it with a promise object.

Closes angular#6731
@btford btford added this to the Backlog milestone Mar 27, 2014
@btford btford removed the gh: issue label Aug 20, 2014
gkalpak added a commit to gkalpak/angular.js that referenced this issue 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 gkalpak mentioned this issue Feb 16, 2018
Closed
gkalpak added a commit to gkalpak/angular.js that referenced this issue 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 issue 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!
```
Narretz pushed a commit to Narretz/angular.js that referenced this issue 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 issue May 11, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
component: ngResource frequency: low severity: confusing
Projects
None yet
Milestone
Backlog - won't fix
Development

Successfully merging a pull request may close this issue.

fix($resource): fix interceptors and success/error callbacks gkalpak/angular.js
4 participants
@btford @alexsanford @g00fy- @lefos987