HTTP_INTERCEPTORS are reset when a lazy loaded module imports another module importing HttpClientModule
thomashilzendegen opened this issue · 35 comments
I'm submitting a...
[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#questionCurrent behavior
The HTTP_INTERCEPTORS provider token is reset when a lazy loaded module imports another module which imports the HttpClientModule by itself.
Expected behavior
The HTTP_INTERCEPTORS from the parent injector should be used.
Minimal reproduction of the problem with instructions
https://plnkr.co/edit/up8P57jwD3lUAZ9WyMS4
What is the motivation / use case for changing the behavior?
It seems that this is currently by design that a parent injector is not asked for the already registered providers from a multi-provider token. In my opinion it is not uncommon to have some kind of shared module which is also importing the HttpClientModule (because it want's to issue HTTP calls, but should not depend on having the module imported in the app module).
Is there any other way to implement this scenario?
Likely relates also to #18894 in some kind.
Environment
Angular version: 5.0.2Before you can use the HttpClient, you need to install the HttpClientModule which provides it. This can be done in your application module, and is only necessary once.
You should import the HttpClientModule only once, see docs
@realappie Sure, but this is quite uncommon that the application module imports a module which has no forRoot() method just because another module uses it (which could be some kind of library etc.). I would expect that this module imports the HttpClientModule by itself and does not depend on a one-time import in the application module.
@thomashilzendegen ... then HttpClientModule is loaded on lazy module level ... and without forRoot() it will be instantiated for each lazy loaded module = reset from your point of view.
@mlc-mlapis Yep, that's true, but there is no forRoot() - so is it a bug or is it intended? And is there any other solution for that, beside You must not import HttpClientModule anywhere else but in the app module? In that case it should also be documented in that way.
@thomashilzendegen ... actually it is intended ... because in 99% cases when you need AJAX communication with a server you probably need it also directly in your main app ... and not only in one or two lazy loaded modules.
But you are right that HttpClientModule with forRoot() would be more general ... so you can create a new request for a new feature ... but in the actual moment I am not sure about the level of a priority ...
@mlc-mlapis The funny thing is that this breaks currently, I have the HttpClientModule in my app module (see the Plunker), but when a lazy loaded module imports a module that imports the HttpClientModule, too, the interceptors are gone. This is because that multi-providers don't look at the injector inheritance, I guess.
@thomashilzendegen same here
@mlc-mlapis @realappie at least the docs should be updated to warn about importing the HttpClientModule more than once, e.g. also in a lazy-loaded module, and add a section about this issue as a possible reason why an interceptor might not be loaded.
@macjohnny Actually, it isn't imported in the lazy-loaded module directly. The lazy-loaded module just uses a module which imports the HttpClientModule. And I think this is the dangerous part, you may not know that this module imports the HttpClientModule when its not your code, for example (without having a look).
@thomashilzendegen, @macjohnny ... a standard structure is to access HttpClientModule only through DI. There isn't probably actually any real case to access it by any other way and repeat the importing. I don't know any, at least.
@mlc-mlapis i am sorry but i can‘t follow your argument.
I think importing some module that itself imports the HttpClientModule is a valid use case.
In fact we faced exactly this situation, which is why we found this issue...
@macjohnny ... it could happen of course but still ... can you describe that scenario a bit to understand how it was happened as a real case?
@mlc-mlapis in our app module we added a provider for our custom interceptor.
In our lazy-loaded feature module we imported the HttpClientModule, and in result our custom interceptor was not provided in the feature module.
The solition was to remove the httpclientmodule import in the feature module.
As @thomashilzendegen stated above, the situation is even harder to figure out when importing some other module in the festure module, while this other module imports the HttpClientModule
@macjohnny ... thanks. Yes, it is a hidden = that's why a dangerous part. It should be notified more explicitly in the doc.
But from the app architecture I still can't see any reason to import HttpClientModule except the main app module and always use DI in any other module. So importing the module somewhere else is probably a mistake from 99%.
@mlc-mlapis i agree that it is a mistake to import the httpclientmodule in any module other than the app module, however, providing a forRoot() method with all providers would prevent this situation. Alternatively, the httpclientmodule could throw an error, since, as you stated, it is a mistake. But silently allowong it withiut any warning can lead to hard-to-find effects.
Especially if you are including e.g. A library which imports the httpclientmodule, it is dangerous.
Issues where exactly this has happened before:
intended behavior.
@mhevery Ok, maybe just with some more info/explanation how this should be handled when its intended? Just saying intended behavior and closing isn’t quite satisfying for all having this issue.
Is the tenor to only import the HttpClientModule in the bootstrapping module and never anywhere else? It would be kind to confirm this at least. :)
Thanks for your help.
I have third-party code that imports HttpClientModule. So i'm just screwed now?
One example is swagger-codegen which imports HttpClientModule. Of course, I can change the code generator in this case, but the current behavior is generally a pitfall.
@JohannesHoppe ... they made a mistake so they have to correct their third-party lib, what else to say. The API should be to inject HttpClient through DI.
@JohannesHoppe I think this issue does not apply to swagger-codegen, since the ApiModule can only be imported once, and since it has a forRoot() method, this should be done in an AppModule / CoreModule only. Or do you think this is still something to be changed in the code generation template? If so, please open an issue
However, I agree with you that it would be nice if the mistake of importing the HttpClientModule anywhere else than in an AppModule / CoreModule, would cause a notice or an error to be presented to the developer.
@macjohnny I would expect that I'm supposed to import ApiModule in my lazy-loaded childs, and ApiModule.forRoot() in my AppModule. In both cases HttpClientModule is imported, which triggers the behaviour that is described in this issue. Isn't it?
@JohannesHoppe in the case of the swagger codegen generated ApiModule: yes, if you import ApiModule in your Lazy-loaded children, then you get the behavior described in this issue.
However, it is not necessary to do so, because it does not add any functionality, as the ApiModule only defines providers of API services, but does not declare any components or anything else that would require importing the ApiModule anywhere else than in the root module. Further, it is explicitly being checked whether the ApiModule was imported more than once and if so, it throws an error.
This means that the behavior of this issue only occurs when you import ApiModule in a lazy-loaded child module only, and not in the AppModule.
the di of router behavior is strange
so as I understand it, it's intended behavior that the HttpClientModule can be imported per module, resulting in the ability to provide individual interceptors per HttpClient without impacting the others. If you'd like to share the same interceptors between all modules, you should import it in the app module only and not import it again in the feature modules. Is that correct?
@Mobiletainment ... as I know Alex wants to make a change to avoid that. And also your summary is valid for lazy loaded modules and not for eagerly loaded ones.
Could a warning be provided whenever this happens or some other sort of indication? It is completely unknown when this happens and why. A library (like ngx-markdown which is referenced here) can completely break an app without it being obvious why.
Yep @Jrubzjeknf is right. The behaviour in ngx-markdown was not expected. But I've had some bad time trying to understand why my interceptors were not working in a file that has nothing to do with the markdown but was in the same module (and therefore got a duplicated instance of HttpClient).
A warning at least would be something really nice, it's quite hard to make sure that none of your libs are importing directly HttpClientModule on their own...
Another example of this issue: dimpu/ngx-md#159
It was not easy to track down because our XSRF broke right after we upgraded to Angular 6, and it was not at all obvious that it was due to an updated markdown module which had newly started importing the HttpClientModule, and the initial suspect was an interface change in Angular itself. So it was straight into the weeds till it became known that the module was being imported twice.
I will say I just bumped into this and it was pretty difficult to figure out what my issue was. To make matters more complicated, I have some backend services that do not require authentication and I was very confused why some things were working and some were not, when really, they were all not utilizing the interceptor due to this. I get that this is closed and intentional, but it's hard to debug when you are not aware of it.
Most of my beginning searches on google returns people who failed to register the interceptor as a provider, which wasn't my case.
I agree that HttpClientModule should only be imported in the root of the application and no external libraries.
However, it would be good to provide a mechanism at least at compile to know if a third-party library is breaking such practice. I don't know if there is one now atm. Which could help a vast number of people who have the unfortunate circumstance.
How i found out which library was using it. I installed Augury browser extension and scanned through the imports for the modules which my application used.
Thanks @ndcunningham !
I've just used Augury to check which of my dependencies was importing HttpClientModule. It works great, on either Firefox or Chrome!
In my case, it was Ng2SmartTableModule (via its CellModule and Ng2CompleterModule)
Now what do I do?
- I could provide my interceptor in all the modules of my app. (Would be quite ugly, right ?)
- I could ask Ng2SmartTable makers to remove the HttpClientModule import in their module. (It would take some time I guess, if they ever agree to do it)
- other solutions ?
For now, I guess I have to stick with the first solution.
No 2 is the only way to go, No 1 as a workaround until it's fixed. I'm not aware of any other solution too.
@alfredott
What we had to do is remove the HttpClientModule import from the library and only import in the application consuming the library.
Though in your scenario a third party is maintaining the library. So, In my opinion, it would be best to raise a problem with them and do the No.1 Solution as a workaround until it has been addressed.
This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.
Read more about our automatic conversation locking policy.
This action has been performed automatically by a bot.