Problem with import of HttpClientModule overwriting interceptors

[karptonite]

I'm submitting a ... (check one with "x")

[x] bug report => search for a similar issue before submitting
[ ] feature request
[ ] question

Versions

@angular/cli: 1.4.7
node: 8.7.0
os: darwin x64
@angular/animations: 5.0.0-rc.2
@angular/common: 5.0.0-rc.2
@angular/compiler: 5.0.0-rc.2
@angular/core: 5.0.0-rc.2
@angular/forms: 5.0.0-rc.2
@angular/http: 5.0.0-rc.2
@angular/platform-browser: 5.0.0-rc.2
@angular/platform-browser-dynamic: 5.0.0-rc.2
@angular/platform-server: 5.0.0-rc.2
@angular/router: 5.0.0-rc.2
@angular/cli: 1.4.7
@angular/compiler-cli: 5.0.0-rc.2
@angular/language-service: 5.0.0-rc.2
typescript: 2.5.3

Repro steps

import ShareDirectiveModule in a module (I've only seen it in a lazy-loaded module, but I don't think it is required), when HTTP_INTERCEPTORS are provided at a higher level (for example, in the root app module).

The Issue

The ShareDirectiveModule imports HttpClientModule. According to the angular docs, the HttpClientModule need only be imported once, in the root of the App. One consequence of re-importing the HttpClientModule is that HTTP_INTERCEPTORS are essentially reset both in the ShareDirectiveModule (which isn't a problem) and for the module which imports the ShareDirectiveModule.

In my App, for example, I found that all of the HttpRequests that were initiated in the module that also imported your module (but nothing at a higher level) ran without my interceptors.

I'm not sure if this is the intended behavior for angular, or if it is an angular bug, but there might be an easy fix: instead of having ShareDirectiveModule import HttpClientModule, amend the docs to have users import HttpClientModule when they import the forRoot() for your module. You may want to do the same with the HttpClientJsonpModule, which adds an interceptor--I'm not sure if it is necessary, though.

Of course, that means that all of your requests will also pass through my interceptor, but it is my responsibility to write the interceptors in a way that doesn't cause problems for your requests.

Obviously, this is a breaking change, but as the package is written now, it is a problem for users who rely on HTTP_INTERCEPTORS.

[MurhafSousli]

Hi @karptonite, The library imports HttpClientModule and HttpClientJsonpModule for internal use but it does not export them, therefore it should not reset your configuration!

For instance, the current demo uses ngx-progress where custom HTTP_INTERCEPTORS are used to start and end the progress, yet It still works fine with the share buttons.

We need a reproduction for the issue before we try to fix it, can you reproduce the issue using this stackblitz, the library is already loaded.

[karptonite]

I'm working on a repro now.

[karptonite]

This was a bit involved for stackblitz, but here is a repro:
https://github.com/karptonite/sharebuttons-conflict
Just clone the repro, run yarn, then yarn start, then load up localhost://4200.

If you click the submodule, you'll see that a book title is not loaded in the submodule, because the necessary interceptor is not run. If you remove the import of ShareButtonsModule from submodule.module.ts, you'll see that that fixes the bug.

Regarding only importing for internal use, I don't believe that is what happens when you import modules that have providers--providers are all global, so if you re-provide for HttpClient (which is, I think, what HttpClientModule does, than the module that imports your module will use that provider.

I don't actually know, though, whether wiping out the HTTP_INTERCEPTORS is intentional, or some sort of bug in Angular.

[karptonite]

Thinking about this further, if you want to be able to use HttpClient without any interference with anything outside of the package, and vice versa, you might need your own provider, your own InjectionToken, that isolates your version of HttpClient from the rest of the app. I don't know how you'd go about that--I doubt it would be trivial.

[karptonite]

I don't think this is necessarily the best way to handle this, but you might take a look at this:
https://github.com/ngx-translate/http-loader

[MurhafSousli]

@karptonite I am checking it now

[MurhafSousli]

@karptonite
Don't name your custom http module as CoreModule. instead, rename it to CustomHttpModule
then, in your lazy module just add your CustomHttpModule

In your lazy module

@NgModule({
  imports: [
    CommonModule, 
    RouterModule.forChild(routes), 
    CustomHttpModule,     // <= Add your custom http module
    ShareButtonsModule
  ],
  // ...

Problem solved!

[naveedahmed1]

The issue still persists, this approach results in multiple instances of the HttpClient. I think it can only be resolved if you make this plugin independent of HttpClient i.e. neither import HttpClientModule nor add HttpClient to providers list in your plugin. In documentation ask users to import HttpModule in their root module.

[karptonite]

@naveedahmed1 @MurhafSousli Yes, I think this issue should be reopened. Importing HttpClientModule in each lazy loaded module is contrary to its intended use, and not an appropriate solution to this problem. The whole point of the CoreModule is that it handles providers (and modules that handle providers) that need only be imported once. See https://angular.io/guide/http#setup-installing-the-module

This is something that you can solve by having the user pass a HttpClient instance as done here, https://github.com/ngx-translate/http-loader, or find a way to create inject an isolated instance of HttpClient with its own InjectionToken (I think that would be preferable).

However, I wouldn't suggest that asking users to import HttpModule as a solution--HttpModule is deprecated, I believe.

[MurhafSousli]

@karptonite @naveedahmed1 Alright, but I need some time to investigate about this

[naveedahmed1]

@MurhafSousli I hope you are doing well, do you have any update to share?

[MurhafSousli]

@naveedahmed1 probably next week

[naveedahmed1]

Sounds great 😊

[naveedahmed1]

Meanwhile I have gone through the code, and I think we can safely remove the HttpClientModule import for this plugin. Whats your conclusion @MurhafSousli ?

[MurhafSousli]

@naveedahmed1 Alright, let's remove it

[naveedahmed1]

Awesome!

[karptonite]

That doesn't entirely solve the problem. HttpClientJsonpModule will now be adding an interceptor to the main application's HttpClient, unless you remove that as well.

[naveedahmed1]

Yes you are right, I would suggest removing that as well, BTW can't we remove the dependency on HttpClientJsonpModule ?

[karptonite]

Even without HttpClientJsonpModule, it is not ideal, since if the main application has interceptors, the requests from ngx-sharebuttons will pass through those interceptors, but it is much better to have that than to interfere with the main application.

[naveedahmed1]

But normally with interceptors, when we have to perform some additional processing on request/response we should check if the request is being sent to our own server or to some external server. For example if we are appending jwt token, we should make sure its not going to a third part server. (I assume this was your concern).

[karptonite]

Yeah, of course that is how we should write interceptors. I'm just saying that having a user's interceptors breaking the package (if they are not well written, or do something unexpected) could be a confusing source of bugs. But again, much better than messing with the interceptors themselves.

[naveedahmed1]

Agreed!

[MurhafSousli]

@karptonite HttpClientModule is removed now
@naveedahmed1 HttpClientJsonpModule is optional now, you only need it for linkedIn and tumblr count

HttpClientModule, // required
HttpClientJsonpModule, // optional
ShareButtonsModule.forRoot()

[karptonite]

Thanks! A definite improvement!

[naveedahmed1]

Great! Thank you so much @MurhafSousli :)

[naveedahmed1]

I think this issue which was fixed in 4.1.0, now appears again in 6.0.0. @MurhafSousli can you please take a look?

[MurhafSousli]

@naveedahmed1 The code has not changed! the plugin does not import the HttpClientModule at all

ngx-sharebuttons/projects/core/src/lib/share.module.ts

Lines 13 to 22 in 27ce548

	@NgModule({
	declarations: [
	ShareButtonDirective,
	ShareCountPipe
	],
	exports: [
	ShareButtonDirective,
	ShareCountPipe
	]
	})

[naveedahmed1]

I just found this:

https://github.com/MurhafSousli/ngx-sharebuttons/blob/master/projects/buttons/src/lib/share-buttons.module.ts

Its importing HttpClientModule , may be its causing the issue.

[MurhafSousli]

That's right! I opened an issue for this #297

[naveedahmed1]

Awesome, thanks, I hope this can be fixed soon :)

[naveedahmed1]

Fixed, this was real quick , great job bro!