feat($resource): add support for `request` and `requestError` interceptors #15674

gkalpak · 2017-02-03T13:56:05Z

Just a rebased version of #13273.

googlebot · 2017-02-03T13:56:07Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-02-03T13:56:07Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

petebacondarwin

One question and one minor suggestion.

petebacondarwin · 2017-12-06T13:01:29Z

test/ngResource/resourceSpec.js

+        $httpBackend.flush();
+        expect(successSpy).toHaveBeenCalledOnce();
+        expect(failureSpy).not.toHaveBeenCalled();
+      });


Do you need a $httpBackend.verifyNoOutstandingRequest(); here to check that the request is actually made or will the expect(...) cause an error if the spec completes without this being requested?

There is a verifyNoOutstandingExpectation() after each test to verify that the request was made.
But I think you are right, we could be more specific in our expectations (e.g. verify call args).

Did I ever mention that I don't like before/afterEach :-P

petebacondarwin · 2017-12-06T13:05:53Z

src/ngResource/resource.js

+              resolve(httpConfig).
+              then(requestInterceptor).
+              catch(requestErrorInterceptor).
+              then($http);


Interested to know why you "reimplemented" request interceptors here rather than modifying the httpConfig to add the interceptors to the request httpConfig (or a clone of it) and rely upon $http to deal with the intercepting.

Currently $http does not support per-request interceptors. Are you suggesting add that as a new feature to $http (and then use it to implement $resource request interceptors?

This would actually be my preferred solution, as the resource interceptors do not work 100% like httpInterceptors anyway (see #11409. Note that even though #13273 is linked in this issue as a solution, adding request/Error interceptors does not actually solve the issue); and because http transforms do not support changing all variables (headers cannot be changed, see #12095)
So you would be able to set interceptors in the http config and httpInterceptors in ngResource.

Obviously this would be a much bigger change which would make http much more complex.

This would actually be my preferred solution, as the resource interceptors do not work 100% like httpInterceptors anyway

$resource response interceptors are special, so indeed do not work exactly as $http interceptors. But request interceptors work pretty much the same afaict.

So you would be able to set interceptors in the http config and httpInterceptors in ngResource.
Obviously this would be a much bigger change which would make http much more complex.

I agree with both observations 😉

If we go with requestInterceptors (fine with me), then we should take this opportunity to document responseInterceptors of resource and the differences to http behavior, preferrably in the same or a follow up PR.

I didn't realise that we couldn't do per-request interceptors in $http!

@gkalpak I'll have to check, but since #11409 is still open I assumed the docs are not clear, or a workaround for this is not covered by the docs.

Just saw there's an extra issue for the inconsistency in response intereceptors: #9334 (also in 1.7)

#9334, is about inconsistency between response/responseError interceptors in $resource itself (not about inconsistency against $http interceptors). #9334 has valid points (that are probably still worth doing something about for 1.7), but is not related to request/requestError interceptors (which are doing the "right thing"™ afaict).

Yeah, it's not directly related. I just thought it could be a good time to clarify any docs in general. But it's not a must.

gkalpak · 2017-12-07T10:09:50Z

PTAL.

petebacondarwin

LGTM

…ptors This commit adds `request` and `requestError` interceptors for `$resource`, as per the documentation found for `$http` interceptors. It is important to note that returning an error at this stage of the request - before the call to `$http` - will completely bypass any global interceptors and/or recovery handlers, as those are added to a separate context. This is intentional; intercepting a request before it is passed to `$http` indicates that the resource itself has made a decision, and that it accepts the responsibility for recovery. Closes angular#5146 BREAKING CHANGE: Previously, calling a `$resource` method would synchronously call `$http`. Now, it will be called asynchronously (regardless if a `request`/`requestError` interceptor has been defined. This is not expected to affect applications at runtime, since the overall operation is asynchronous already, but may affect assertions in tests. For example, if you want to assert that `$http` has been called with specific arguments as a result of a `$resource` call, you now need to run a `$digest` first, to ensure the (possibly empty) request interceptor promise has been resolved. Before: ```js it('...', function() { $httpBackend.expectGET('/api/things').respond(...); var Things = $resource('/api/things'); Things.query(); expect($http).toHaveBeenCalledWith(...); }); ``` After: ```js it('...', function() { $httpBackend.expectGET('/api/things').respond(...); var Things = $resource('/api/things'); Things.query(); $rootScope.$digest(); expect($http).toHaveBeenCalledWith(...); }); ```

…interceptors

googlebot added the cla: no label Feb 3, 2017

gkalpak added component: ngResource frequency: low needs: breaking change severity: inconvenient type: feature labels Feb 3, 2017

gkalpak added this to the Backlog milestone Feb 3, 2017

gkalpak mentioned this pull request Feb 3, 2017

Add request and requestError interception in ngResource #13273

Closed

gkalpak added the needs: review label Feb 4, 2017

gkalpak modified the milestones: 1.7.0, Backlog Feb 4, 2017

Narretz mentioned this pull request Nov 1, 2017

ngResource interceptors called inconsistently #9334

Closed

gkalpak force-pushed the rebase-13273 branch 3 times, most recently from 4ca68a2 to 0416afe Compare December 5, 2017 14:44

petebacondarwin suggested changes Dec 6, 2017

View reviewed changes

petebacondarwin approved these changes Dec 8, 2017

View reviewed changes

petebacondarwin added needs: merging and removed needs: review labels Dec 11, 2017

Michael Krotscheck and others added 2 commits December 18, 2017 14:14

fixup! feat($resource): add support for request and requestError …

173db93

…interceptors

gkalpak force-pushed the rebase-13273 branch from 82196c4 to 173db93 Compare December 18, 2017 12:15

petebacondarwin merged commit 240a3dd into angular:master Dec 18, 2017

gkalpak deleted the rebase-13273 branch January 3, 2018 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($resource): add support for `request` and `requestError` interceptors #15674

feat($resource): add support for `request` and `requestError` interceptors #15674

gkalpak commented Feb 3, 2017

googlebot commented Feb 3, 2017

googlebot commented Feb 3, 2017

petebacondarwin left a comment

petebacondarwin Dec 6, 2017

gkalpak Dec 6, 2017

petebacondarwin Dec 8, 2017

gkalpak Dec 11, 2017

petebacondarwin Dec 6, 2017

gkalpak Dec 6, 2017

Narretz Dec 7, 2017

gkalpak Dec 7, 2017

Narretz Dec 7, 2017

petebacondarwin Dec 8, 2017

Narretz Dec 8, 2017

Narretz Dec 8, 2017 •

edited

Loading

gkalpak Dec 11, 2017

Narretz Dec 11, 2017

gkalpak commented Dec 7, 2017

petebacondarwin left a comment

feat($resource): add support for request and requestError interceptors #15674

feat($resource): add support for request and requestError interceptors #15674

Conversation

gkalpak commented Feb 3, 2017

googlebot commented Feb 3, 2017

googlebot commented Feb 3, 2017

petebacondarwin 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

Narretz Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Dec 7, 2017

petebacondarwin left a comment

Choose a reason for hiding this comment

feat($resource): add support for `request` and `requestError` interceptors #15674

feat($resource): add support for `request` and `requestError` interceptors #15674

Narretz Dec 8, 2017 •

edited

Loading