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

Can you move HttpModule in MdIconModule to component scope (or don't import it)? #2616

Closed
mrpachara opened this issue Jan 12, 2017 · 8 comments · Fixed by #3792
Closed

Can you move HttpModule in MdIconModule to component scope (or don't import it)? #2616

mrpachara opened this issue Jan 12, 2017 · 8 comments · Fixed by #3792
Assignees
Labels
help wanted The team would appreciate a PR from the community to address this issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@mrpachara
Copy link

Bug, feature request, or proposal:

Bug

What is the current behavior?

The feature modules that import MaterialModule (import MdIconModule) will create separated http provider and override the application-wide customized Http.

What are the steps to reproduce?

The following link shows the problem.
http://plnkr.co/edit/hDLOuvRPWxPIY3TwX4GB?p=preview

What is the use-case or motivation for changing an existing behavior?

Cannot use application-wide customized Http in feature module that import MaterialModule

Which versions of Angular, Material, OS, browsers are affected?

@angular@2.4.2
@angular/material@2.0.0-beta.1
Google Chrome Version 57.0.2970.0 (Official Build) dev (64-bit)
Window 10

@jelbourn
Copy link
Member

@alxhub do you have any input on how a component that depends on Http should depend on HttpModule WRT this issue?

@petertangelder
Copy link

petertangelder commented Feb 16, 2017

I'm facing the same issue. I created a custom http service:

@Injectable() export class SecureHttp extends Http { ... }

I provide this service in app.module.ts:

providers: [ { provide: Http, useClass: SecureHttp } ],

If I don't use Material Design, this SecureHttp service is used in all classes that inject the Http service. But if I do include MaterialModule in other modules as well, the default Http service is used instead of my SecureHttp, because the HttpModule is used in MdIconModule. I can fix this by adding
providers: [ { provide: Http, useClass: SecureHttp } ],
in each module.

Is this intended behavior?

@jelbourn
Copy link
Member

Had some discussion w/ @alxhub about this. The idea is to change md-icon to optionally inject Http and throw an error if it isn't provided without directly importing the HttpModule itself.

@jelbourn jelbourn added help wanted The team would appreciate a PR from the community to address this issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed discussion labels Mar 10, 2017
@eladh
Copy link

eladh commented Mar 20, 2017

HI @jelbourn , @EladBezalel

Upon your last comment, I thought about this logic.
remove of direct importing of HttpModule and in icon-registry get the Injector and check
if there already Http implementation supplied, if so use it or use the ReflectiveInjector to create the default one.

  private getHttp() {
    let httpService: Http = this._injector.get(Http ,null);

    if (!httpService) {
      let providers = (<any>HttpModule).decorators[0].args[0].providers;
      let injector = ReflectiveInjector.resolveAndCreate(providers);
      httpService = injector.get(Http);
    }

    return httpService;
  }
}

@crisbeto crisbeto self-assigned this Mar 26, 2017
crisbeto added a commit to crisbeto/material2 that referenced this issue Mar 26, 2017
Makes `@angular/http` an optional dependency since it is only being used by the icon module. An error will be thrown only if the user runs into something that requires the `HttpModule`.

Fixes angular#2616.
crisbeto added a commit to crisbeto/material2 that referenced this issue Mar 26, 2017
* Makes `@angular/http` an optional dependency since it is only being used by the icon module. An error will be thrown only if the user runs into something that requires the `HttpModule`.
* Moves the various icon error classes into `icon-errors.ts`.
* Moves the icon registry provider next to the registry itself, instead of in the same file as the icon directive.

Fixes angular#2616.
crisbeto added a commit to crisbeto/material2 that referenced this issue Mar 26, 2017
* Makes `@angular/http` an optional dependency since it is only being used by the icon module. An error will be thrown only if the user runs into something that requires the `HttpModule`.
* Moves the various icon error classes into `icon-errors.ts`.
* Moves the icon registry provider next to the registry itself, instead of in the same file as the icon directive.

Fixes angular#2616.
@jelbourn
Copy link
Member

jelbourn commented Apr 8, 2017

Done in #3792

@jelbourn jelbourn closed this as completed Apr 8, 2017
crisbeto added a commit to crisbeto/material2 that referenced this issue Apr 12, 2017
* Makes `@angular/http` an optional dependency since it is only being used by the icon module. An error will be thrown only if the user runs into something that requires the `HttpModule`.
* Moves the various icon error classes into `icon-errors.ts`.
* Moves the icon registry provider next to the registry itself, instead of in the same file as the icon directive.

Fixes angular#2616.
crisbeto added a commit to crisbeto/material2 that referenced this issue Apr 28, 2017
* Makes `@angular/http` an optional dependency since it is only being used by the icon module. An error will be thrown only if the user runs into something that requires the `HttpModule`.
* Moves the various icon error classes into `icon-errors.ts`.
* Moves the icon registry provider next to the registry itself, instead of in the same file as the icon directive.

Fixes angular#2616.
crisbeto added a commit to crisbeto/material2 that referenced this issue Apr 29, 2017
* Makes `@angular/http` an optional dependency since it is only being used by the icon module. An error will be thrown only if the user runs into something that requires the `HttpModule`.
* Moves the various icon error classes into `icon-errors.ts`.
* Moves the icon registry provider next to the registry itself, instead of in the same file as the icon directive.

Fixes angular#2616.
crisbeto added a commit to crisbeto/material2 that referenced this issue Apr 29, 2017
* Makes `@angular/http` an optional dependency since it is only being used by the icon module. An error will be thrown only if the user runs into something that requires the `HttpModule`.
* Moves the various icon error classes into `icon-errors.ts`.
* Moves the icon registry provider next to the registry itself, instead of in the same file as the icon directive.

Fixes angular#2616.
andrewseguin pushed a commit that referenced this issue May 2, 2017
* Makes `@angular/http` an optional dependency since it is only being used by the icon module. An error will be thrown only if the user runs into something that requires the `HttpModule`.
* Moves the various icon error classes into `icon-errors.ts`.
* Moves the icon registry provider next to the registry itself, instead of in the same file as the icon directive.

Fixes #2616.
@csvn
Copy link
Contributor

csvn commented Jun 5, 2017

If I install @angular/material and uninstall @angular/http in a fresh cli project, ng serve wont compile. Is this by design? I expected that the http package did not need to be installed due to it being removed from peerDependencies in 3792.

(I get similar errors with @angular/animations. Only core/common packages are in peerDependencies, even though TS/Webpack require http and animations too).

@jelbourn
Copy link
Member

jelbourn commented Jun 5, 2017

@csvn please file a new issue

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted The team would appreciate a PR from the community to address this issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants