Instantiate interceptor only once #121
Conversation
lib/httpMock.js
Outdated
| var interceptor = angular.isString(interceptorExpression) ? | ||
| $injector.get(interceptorExpression) : $injector.invoke(interceptorExpression); | ||
| interceptors.push(interceptor); | ||
| }); |
There was a problem hiding this comment.
I think it'd be cleaner to keep the if/else block rather than use a ternary operator.
lib/httpMock.js
Outdated
| return $injector.invoke(interceptorExpression); | ||
| } | ||
| } | ||
| angular.forEach($httpProvider.interceptors, function(interceptorExpression) { |
There was a problem hiding this comment.
I'm not sure if it happens, but if interceptors are added to $httpProvider.interceptors after this loop runs then they'll be ignored.
There was a problem hiding this comment.
Maybe. I can not answer this question.
However we had problems with angular-loading-bar and protractor-http-mock working together. The root cause was that a stateful interceptor by anonymous factory which caused troubles with protractor-http-mock. It is hard to blame and fix one of each side in this case.
If the interceptor uses a named provider, the $injector can cache and reuse the instance and this PR can be dropped. But this issue is hard to track down and it would be nice if it does not occur in the future.
There was a problem hiding this comment.
Regarding added interceptors: This case should only happen if the $httpProvider is modified after the mock() call in a jasmine test scenario. The $http service can not modify interceptors. I think the normal case is to mock the $http service at the very and of a test scenario in a beforeEach() block. Therefore the mock is the last delegate of the $http service, isn't it?
And than this PR should be safe.
|
Nice first PR @xemle! I noticed that your commit isn't linked to your GitHub profile:
I think you just need to add the email address associated with the commit to your profile. See https://help.github.com/articles/why-are-my-contributions-not-showing-up-on-my-profile/#you-havent-added-your-local-git-commit-email-to-your-profile. The email can be seen in git, or on GitHub here: https://github.com/atecarlos/protractor-http-mock/commit/b799952e96bd8868e6045b91544d3badb10ac8b1.patch I realise this sounds like I'm a maintainer, but I'm not; I just really like this project and got a notification for it =) |
|
Hi @xemle. Thank you very much for supporting this project with this PR. We've had issues with angular loading bar for a long time now, but I just never had the time to dive deep into it and find the cause. This may be it! However, I'm hesitant on merging this PR just yet until I fully understand the consequences of this - I would hate to affect users that are somehow depending on a particular order or processing for the interceptors. I need to refresh my memory on how Angular itself works. I assume that since some projects use singleton interceptors, then the interceptors must be evaluated once in the lifetime of the app - as in your PR, and not once per request - as in the current functionality. In the mean time, could you write a unit test for this scenario? A feature this important requires a unit test, and also, an example Thanks! |
|
Looking at the source code for Angular here: https://github.com/angular/angular.js/blob/master/src/ng/http.js It seems that what you are doing in this PR is correct. With that said, I would like to have the tests added before we merge this in. |
|
@atecarlos Thank you for investigating this PR. I've added a unit test case with stateful anonymous interceptor. This test breaks in the (unfixed) master. Regarding While this PR fixes #112, I hope that my PR angular-loading-bar#365 (Avoid multiple interceptor instances due factory) will be accepted, to fix the other side, too :-) |
|
Hi @xemle . Sorry for the delay on this. I'm out of the country for a couple of weeks. I'll get to this as soon as I'm back. |
|
Merged and published as 0.10.0. Thanks! |
|
Thanks a lot! |
If an interceptor is a anonymous factory (e.g. via
$httpProvider.interceptors.push(['$q', function($q) {...}])), the interceptor is created multiple times during the test life cycle. If such interceptor is a stateful singleton like the Angular Loading Bar, the state is ambiguous.The PR fixes the problem by instantiating the interceptors once and reuses the instances.