New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP_INTERCEPTORS are reset when a lazy loaded module imports another module importing HttpClientModule #20575

Closed
thomashilzendegen opened this Issue Nov 22, 2017 · 28 comments

Comments

Projects
None yet
@thomashilzendegen

thomashilzendegen commented Nov 22, 2017

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#question

Current 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.2
@realappie

This comment has been minimized.

Show comment
Hide comment
@realappie

realappie Nov 22, 2017

Before 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 commented Nov 22, 2017

Before 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

@thomashilzendegen

This comment has been minimized.

Show comment
Hide comment
@thomashilzendegen

thomashilzendegen Nov 22, 2017

@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 commented Nov 22, 2017

@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.

@mlc-mlapis

This comment has been minimized.

Show comment
Hide comment
@mlc-mlapis

mlc-mlapis Nov 22, 2017

@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 commented Nov 22, 2017

@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.

@thomashilzendegen

This comment has been minimized.

Show comment
Hide comment
@thomashilzendegen

thomashilzendegen Nov 22, 2017

@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 commented Nov 22, 2017

@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.

@mlc-mlapis

This comment has been minimized.

Show comment
Hide comment
@mlc-mlapis

mlc-mlapis Nov 22, 2017

@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 commented Nov 22, 2017

@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 ...

@thomashilzendegen

This comment has been minimized.

Show comment
Hide comment
@thomashilzendegen

thomashilzendegen Nov 22, 2017

@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 commented Nov 22, 2017

@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.

@svenhuber

This comment has been minimized.

Show comment
Hide comment
@svenhuber

svenhuber commented Nov 22, 2017

@thomashilzendegen same here

@macjohnny

This comment has been minimized.

Show comment
Hide comment
@macjohnny

macjohnny Nov 22, 2017

@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 commented Nov 22, 2017

@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.

@thomashilzendegen

This comment has been minimized.

Show comment
Hide comment
@thomashilzendegen

thomashilzendegen Nov 22, 2017

@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 commented Nov 22, 2017

@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).

@mlc-mlapis

This comment has been minimized.

Show comment
Hide comment
@mlc-mlapis

mlc-mlapis Nov 22, 2017

@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 commented Nov 22, 2017

@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.

@macjohnny

This comment has been minimized.

Show comment
Hide comment
@macjohnny

macjohnny Nov 22, 2017

@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 commented Nov 22, 2017

@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...

@mlc-mlapis

This comment has been minimized.

Show comment
Hide comment
@mlc-mlapis

mlc-mlapis Nov 22, 2017

@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 commented Nov 22, 2017

@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?

@macjohnny

This comment has been minimized.

Show comment
Hide comment
@macjohnny

macjohnny Nov 22, 2017

@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 commented Nov 22, 2017

@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

@mlc-mlapis

This comment has been minimized.

Show comment
Hide comment
@mlc-mlapis

mlc-mlapis Nov 22, 2017

@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 commented Nov 22, 2017

@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%.

@macjohnny

This comment has been minimized.

Show comment
Hide comment
@macjohnny

macjohnny Nov 22, 2017

@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.

macjohnny commented Nov 22, 2017

@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.

@christianliebel

This comment has been minimized.

Show comment
Hide comment
@christianliebel

christianliebel commented Nov 22, 2017

Issues where exactly this has happened before:

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Nov 28, 2017

Member

intended behavior.

Member

mhevery commented Nov 28, 2017

intended behavior.

@mhevery mhevery closed this Nov 28, 2017

@thomashilzendegen

This comment has been minimized.

Show comment
Hide comment
@thomashilzendegen

thomashilzendegen Nov 29, 2017

@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.

thomashilzendegen commented Nov 29, 2017

@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.

@JohannesHoppe

This comment has been minimized.

Show comment
Hide comment
@JohannesHoppe

JohannesHoppe Jan 7, 2018

Contributor

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.

Contributor

JohannesHoppe commented Jan 7, 2018

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.

@mlc-mlapis

This comment has been minimized.

Show comment
Hide comment
@mlc-mlapis

mlc-mlapis Jan 7, 2018

@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.

mlc-mlapis commented Jan 7, 2018

@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.

@macjohnny

This comment has been minimized.

Show comment
Hide comment
@macjohnny

macjohnny Jan 8, 2018

@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 commented Jan 8, 2018

@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.

@JohannesHoppe

This comment has been minimized.

Show comment
Hide comment
@JohannesHoppe

JohannesHoppe Jan 9, 2018

Contributor

@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?

Contributor

JohannesHoppe commented Jan 9, 2018

@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?

@macjohnny

This comment has been minimized.

Show comment
Hide comment
@macjohnny

macjohnny Jan 9, 2018

@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.

macjohnny commented Jan 9, 2018

@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.

macjohnny added a commit to macjohnny/swagger-codegen that referenced this issue Jan 11, 2018

#7354: prevent angular/angular#20575 (comment) add error message abou…
…essage about importing the HttpModule / HttpClientModule

wing328 added a commit to swagger-api/swagger-codegen that referenced this issue Jan 20, 2018

[Feature][Angular] improve docs angular import (#7363)
* #7354: improve docs about importing multiple ApiModules, prevent angular/angular#20575 (comment)

* #7354: prevent angular/angular#20575 (comment) add error message about importing the HttpModule / HttpClientModule

* #7354: generate sample files

* #7354: generate README.md also for non-npm packages
@LinBoLen

This comment has been minimized.

Show comment
Hide comment
@LinBoLen

LinBoLen Apr 11, 2018

the di of router behavior is strange

LinBoLen commented Apr 11, 2018

the di of router behavior is strange

@Mobiletainment

This comment has been minimized.

Show comment
Hide comment
@Mobiletainment

Mobiletainment Apr 19, 2018

Contributor

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?

Contributor

Mobiletainment commented Apr 19, 2018

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?

@mlc-mlapis

This comment has been minimized.

Show comment
Hide comment
@mlc-mlapis

mlc-mlapis Apr 19, 2018

@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.

mlc-mlapis commented Apr 19, 2018

@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.

@Jrubzjeknf

This comment has been minimized.

Show comment
Hide comment
@Jrubzjeknf

Jrubzjeknf May 23, 2018

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.

Jrubzjeknf commented May 23, 2018

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.

@maxime1992

This comment has been minimized.

Show comment
Hide comment
@maxime1992

maxime1992 May 23, 2018

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

maxime1992 commented May 23, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment