Do not import `HttpClientModule`

[jfcere]

Do not import `HttpClientModule`

[jfcere]

Look good at first sight ... I'm gonna look at it tonight from home.
Oh and by the way thanks a lot for doing the tests, usually people don't care so much about it 😄

[jfcere]

I did some tests on my side and I can confirm that if you import MarkdownModule into a lazy loaded module it will reset the interceptors because it imports HttpClientModule so it should be removed.

But I also found out that when HttpClientModule is imported into the AppModule it is not necessary to pass HttpClient through MarkdownModule.forRoot() to be provided inside MarkdownModule.

That fact seems to be backed twice in the same issue your pointed out:

• angular/angular#20575 (comment - macjohny)
• angular/angular#20575 (comment - Mobiletainment)

Could you do the same test on your end and confirm that last discovery?

[maxime1992]

@jfcere I've added a commit. Let me know what you think :)

[jfcere]

@maxime1992 thx for reporting this out and the PR.

I am at work right now but I've read the issue quickly and it seems to make sense to remove the import of HttpClientModule inside MarkdownModule. I am going to take a look tonight to be sure it doesn't bring any undesirable side effects and if not i'll accept your PR and release a fix.

I'll keep you in touch!

[maxime1992]

@jfcere cool, I gave a quick try and then struggled to publish my fork on npm all afternoon but turns out it was an official problem with npm...

I'll be able to fully test that tomorrow morning so if you're keen to wait 1/2 day I'll make sur everything's fine.

[maxime1992]

Even though it's not necessary, it enforces the client to pass it and avoid errors.

If somebody does not want to use HttpClientModule in their app and they do not see it imported to be passed to MarkddownModule, they'll just remove it.

IMO it'd be better to keep an explicit import :)

[jfcere]

But it just seems wrong to force somebody to import HttpClientModule and pass HttpClient to MarkdownModule if they don't need any remote content ...

Another idea would be to set @Optional() decorator on HttpClient in MarkdownService constructor here and add an if condition in the getSource method here and throw if it is undefined with a related error message so that if anybody forgot to include HttpClientModule in their application they would be noticed if they use the [src] input property to fetch remote files via http.

What do you think?

[maxime1992]

Yep sounds like a cool idea. I'll give a try.

[jfcere]

Thanks again for your PR and your work around this!

I am going to do some small rework to use the new angular library projects architecture and release v2.0 afterward somewhere around the weekend.

If you really need this quick I can do a v2.0.0-beta.0 release if you want? Although you could probably also pull directly on master branch.

[maxime1992]

You're welcome 😁. Thanks for sharing the lib in the first place!

Sounds like a plan. But careful, as far as I've seen there's no way to continuously rebuild a library yet. So if you make changes and want to see them running in the demo app you'll have to rebuild everytime which is a bit annoying.

It's OK I've published a temporary fork on npm because I really needed that for my CI. But I'll remove it as soon as V2 is here 👍.