Skip to content
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

refactor(ng-lib): manage HttpClient dependency #144

Conversation

LayZeeDK
Copy link
Contributor

@LayZeeDK LayZeeDK commented Jan 5, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

ComponentsModule.forRoot only provides HttpClient, but not its dependencies.

Pull Requests: #69 #75
Issue Number: #52

What is the new behavior?

Instead of providing only HttpClient, but not its dependencies, we now re-export HttpClientModule in the static forRoot method of ComponentsModule.

I have also added a test suite for the ComponentsModule which exercises injecting HttpClient and its dependencies.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Note that the init schematic currently adds HttpClientModule to the consumers AppModule imports.

Sorry for bringing this up again, but the current implementation only provides HttpClient. I did some digging and discovered that HttpClientModule actually provides quite a few dependencies used by HttpClient at runtime.

How do we want to handle this?

In the new approach of this PR, I simply re-export HttpClientModule in ComponentsModule.forRoot.

Usually, we would just expect the consuming application to inject HttpClientModule. The classes depending on HttpClient (ScullyRoutesService and ScullyContentComponent) would crash at runtime, if HttpClient and its dependencies were not provided by the consuming application.

Is this a better option? Then we can remove the static forRoot method instead. If so, we could probably detect the error early by attempting to inject HttpClient in ComponentsModule's constructor as an optional dependency and throw an error with a helpful message if it could not be resolved.

@claassistantio
Copy link

claassistantio commented Jan 5, 2020

CLA assistant check
All committers have signed the CLA.

@LayZeeDK
Copy link
Contributor Author

LayZeeDK commented Jan 9, 2020

Alternatives to guard for HttpClient or have transient import of HttpClientModule.
https://gist.github.com/LayZeeDK/d55b22365c87260387273a42f6baa5cc

@LayZeeDK LayZeeDK changed the title refactor(ng-lib): re-export HttpClientModule refactor(ng-lib): manage HttpClient dependency Jan 9, 2020
@LayZeeDK
Copy link
Contributor Author

Closed because of decision to remove the HttpClient dependency.

@LayZeeDK LayZeeDK closed this Jan 10, 2020
@LayZeeDK LayZeeDK deleted the LayZeeDK/refactor/componentsmodule-re-export-httpclientmodule branch January 10, 2020 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants