fix($resource): fix interceptors and success/error callbacks #16446

gkalpak · 2018-02-10T21:47:21Z

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 #9334 (comment).

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

Fixes #6731
Fixes #9334
Closes #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.:

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

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 · 2018-02-16T10:28:16Z

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.

So that means if I do:

User.get({userId:123}, function successCallback() {
 throw new Error();
}).$promise.then(function(user) {
      $scope.user = user;
    }).catch(function(error) { )}

catch won't be called? I think the commit message should make this clearer. I also don't see any tests for this behavior, is this intentional?

You could also add a note to the (general) commit message that you've replaced use of callbacks in the docs with promise, which is a nice touch

Narretz · 2018-02-16T10:30:37Z

Does this PR obsolete #6865?

gkalpak · 2018-02-16T12:32:46Z

So that means if I do ... catch won't be called?

Yes. It is up for debate, but I think it is reasonable.
I would like to deprecate and discourage the callbacks altogether, but unfortunately there is no (straight forward) alternative if you want to get access to some response metadata (e.g. status, headers, etc).

I think the commit message should make this clearer.

👍

I also don't see any tests for this behavior, is this intentional?

There weren't any tests for the exact behavior before, so I played along. If we agree that this is the behavior we want, I can add tests 😃

Does this PR obsolete #6865?

Yes! (I didn't know about that one. Thx for pointing it out 👍)

Narretz · 2018-02-16T12:38:08Z

If we agree that this is the behavior we want, I can add tests

If find the new behavior very reasonable. A callback isn't (usually) chainable like a promise. So you have my 👍 for adding tests :D

gkalpak · 2018-02-16T14:19:46Z

Added a couple of tests and updated the commit message with some examples.
@Narretz, PTAL

Narretz

Great stuff @gkalpak. Just a few minor change requests. Per instance interceptors would also look good in $http, I wonder if we should implement them.

I think @petebacondarwin should also reviw this.

Narretz · 2018-02-18T17:33:20Z

src/ngResource/resource.js

+ *     global `$http` interceptors. Also, rejecting or throwing an error inside the `request`
+ *     interceptor will result in calling the `responseError` interceptor.
+       The resource instance or collection is available on the `resource` property of the
+ *     `http response` object passed to `response`/`responseError` interceptors.


Can you link here to the example below?

Narretz · 2018-02-18T17:39:56Z

src/ngResource/resource.js

+              $q.reject;
+            var successCallback = onSuccess ? function(val) {
+              onSuccess(val, response.headers, response.status, response.statusText);
+            } : undefined;


Isn't noop better than undefined here? (and in line 773)

It is better for these to be undefined since there is different behaviour in the call to then(...) function if onError is a defined function. But this only would make a difference if we returned the promise that is created at https://github.com/angular/angular.js/pull/16446/files#diff-67ae87630c0e91cba343677ed730daafR861.

As it stands undefined and noop have the same result below.

Narretz · 2018-02-18T17:57:39Z

test/ngResource/resourceSpec.js

-            }
+
+    describe('responseInterceptor', function() {
+      it('should allow per action response interceptor that gets full response', function() {


"full response" here means no headers or statusText, right? Because you don't assert for them. Could we add them if they are missing?

Narretz · 2018-02-18T17:59:44Z

src/ngResource/resource.js

@@ -294,46 +301,54 @@ function shallowClearAndCopy(src, dst) {
 *
 * ### Credit card resource


It would be nice if this example and the next one had names that tell what they are showing, e.g. for this one "Basic Usage" and for the second one maybe "Accessing the http response / Promises and Callbacks"

petebacondarwin · 2018-02-19T14:02:44Z

src/ngResource/resource.js

- *   - **`timeout`** – `{number}` – timeout in milliseconds.<br />
+ *   - **`cache`** – `{boolean|Cache}` – If true, a default `$http` cache will be used to cache the
+ *     GET request, otherwise if a cache instance built with {@link ng.$cacheFactory $cacheFactory}
+ *     is supplied, this cache will be used for caching.


what else would a "cache" be used for? 😛

Good question ¯\_(ツ)_/¯ - I'm just formatting short lines here 😛
I copied the description from $http:

cache – {boolean|Cache} – A boolean value or object created with
{@link ng.$cacheFactory $cacheFactory} to enable or disable caching of the HTTP response.
See {@link $http#caching $http Caching} for more information.

petebacondarwin · 2018-02-19T14:08:31Z

src/ngResource/resource.js

+ *     response interceptors. Make sure you return an appropriate value and not the `response`
+ *     object passed as input. For example, the default `response` interceptor (which gets applied
+ *     if you don't specify a custom one) returns `response.resource`.
+ *   - **`hasBody`** - `{boolean}` - Allows to specify if a request body should be included or not.


Personally I prefer more explicit definitions:

- **`hasBody`** - `{boolean}` - If true, then the request will have a body. If not specified, then only POST, PUT and PATCH requrest will have a body.

petebacondarwin · 2018-02-19T14:09:03Z

src/ngResource/resource.js

- *   var User = $resource('/user/:userId', {userId:'@id'});
- *   var user = User.get({userId:123}, function() {
+ *   var User = $resource('/user/:userId', {userId: '@id'});
+ *   User.get({userId: 123}).$promise.then(function(user) {


petebacondarwin · 2018-02-19T14:18:27Z

src/ngResource/resource.js

-                } :
-                undefined);
+            // Run the `success`/`error` callbacks, but do not let them affect the returned promise.
+            promise.then(successCallback, errorCallback);


So you explicitly choose for the success and error callbacks not to affect the returned promise, thus causing a breaking change.

Is this BC really necessary? Why not allow them to pass through to returned promise, given that this was the previous behaviour?

Just to be clear, this commit will be a BC regardless if we propagate callback errors to $promise or not.

Previously, callbacks did not 100% affect $promise. E.g. their returned value was ignored. It just so happened that throwing was reflected on $promise as a rejection (since it happened inside a then() callback).

Changing the general behavior (e.g. forwarding the value returned by callbacks to $promise) would be unexpected and confusing imo. I am fine with both the current state of the PR as well as having thrown errors propagate to $promise as rejections. (I just chose to not preserve that particular aspect of the previous behavior, because it felt unintentional.)

petebacondarwin · 2018-02-19T14:19:01Z

test/ngResource/resourceSpec.js

@@ -2026,6 +2143,41 @@ describe('handling rejections', function() {
    }
  );

+
+  it('should not propagate exceptions in success callback to the returned promise', function() {


why is it a bad thing for this to happen?

Imo, the success callback should not interfere with the promise chain, since it's strictly just a callback. Error handling should only happen in the interceptors. Do you think this will affect many apps?

I agree that in an ideal world this should be the behaviour - I am just wondering if it is so important that it should cause a BC.

Let's discuss in https://github.com/angular/angular.js/pull/16446/files#r169286982.

gkalpak · 2018-02-20T11:28:54Z

Added another commit. PTAL, @Narretz & @petebacondarwin ❤️

Narretz · 2018-02-20T12:35:28Z

travis only failed because of the templaterequest errors. ;)

@id

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! ```

The sitemap.xml might also prevent the indexing, as the partials are not listed. Related to angular#16432 Closes angular#16457 Closes angular#16446

Closes angular#16446

gkalpak added type: bug component: ngResource frequency: low needs: breaking change severity: broken expected use severity: confusing labels Feb 10, 2018

googlebot added the cla: yes label Feb 10, 2018

gkalpak added this to the 1.7.0 milestone Feb 10, 2018

gkalpak force-pushed the fix-resource-interceptors branch from 14275c2 to 7b23ce5 Compare February 16, 2018 14:16

Narretz suggested changes Feb 18, 2018

View reviewed changes

petebacondarwin reviewed Feb 19, 2018

View reviewed changes

gkalpak force-pushed the fix-resource-interceptors branch from 0f1ab76 to c5f0cfe Compare February 20, 2018 11:27

petebacondarwin approved these changes Feb 20, 2018

View reviewed changes

Narretz approved these changes Feb 20, 2018

View reviewed changes

gkalpak added 3 commits February 20, 2018 15:09

fixup! fix($resource): fix interceptors and success/error callbacks

f962ce2

fixup! fix($resource): fix interceptors and success/error callbacks

89b8ab1

gkalpak force-pushed the fix-resource-interceptors branch from c5f0cfe to cad5a77 Compare February 20, 2018 13:28

fixup! fix($resource): fix interceptors and success/error callbacks

a2df3a1

gkalpak force-pushed the fix-resource-interceptors branch from cad5a77 to a2df3a1 Compare February 20, 2018 13:28

gkalpak closed this in ea05857 Feb 20, 2018

gkalpak deleted the fix-resource-interceptors branch February 20, 2018 17:07

Narretz pushed a commit to Narretz/angular.js that referenced this pull request Feb 23, 2018

fixup! fix($resource): fix interceptors and success/error callbacks

26f485f

Closes angular#16446

dependencies bot mentioned this pull request May 11, 2018

Update 3 dependencies from npm JetBrains/ring-ui#257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($resource): fix interceptors and success/error callbacks #16446

fix($resource): fix interceptors and success/error callbacks #16446

gkalpak commented Feb 10, 2018 •

edited

Loading

Narretz commented Feb 16, 2018

Narretz commented Feb 16, 2018

gkalpak commented Feb 16, 2018

Narretz commented Feb 16, 2018

gkalpak commented Feb 16, 2018

Narretz left a comment

Narretz Feb 18, 2018

Narretz Feb 18, 2018

petebacondarwin Feb 19, 2018

Narretz Feb 18, 2018

Narretz Feb 18, 2018

petebacondarwin Feb 19, 2018

petebacondarwin Feb 19, 2018

gkalpak Feb 20, 2018

petebacondarwin Feb 19, 2018

petebacondarwin Feb 19, 2018

petebacondarwin Feb 19, 2018

gkalpak Feb 20, 2018

petebacondarwin Feb 20, 2018

petebacondarwin Feb 19, 2018

Narretz Feb 19, 2018

petebacondarwin Feb 19, 2018

gkalpak Feb 20, 2018

gkalpak commented Feb 20, 2018

Narretz commented Feb 20, 2018

		@@ -294,46 +301,54 @@ function shallowClearAndCopy(src, dst) {
		*
		* ### Credit card resource

fix($resource): fix interceptors and success/error callbacks #16446

fix($resource): fix interceptors and success/error callbacks #16446

Conversation

gkalpak commented Feb 10, 2018 • edited Loading

Narretz commented Feb 16, 2018

Narretz commented Feb 16, 2018

gkalpak commented Feb 16, 2018

Narretz commented Feb 16, 2018

gkalpak commented Feb 16, 2018

Narretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Feb 20, 2018

Narretz commented Feb 20, 2018

gkalpak commented Feb 10, 2018 •

edited

Loading