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

Make Material modules compatible with lazy-loaded components #1071

Closed
RoxKilly opened this issue Aug 18, 2016 · 5 comments
Closed

Make Material modules compatible with lazy-loaded components #1071

RoxKilly opened this issue Aug 18, 2016 · 5 comments
Assignees
Labels
P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful
Milestone

Comments

@RoxKilly
Copy link

RoxKilly commented Aug 18, 2016

Bug, feature request, or proposal:

Proposal

What is the expected behavior?

Services that are only practically useful as singletons should be available as such. MdIconRegistry is a perfect example. SinceMdIconRegistry.addSvgIconSet() adds icons that should be available app-wide, this service is most useful as a singleton. Instead of simply offering MdIconModule as an export, offer MdIconModule.forRoot as an alternative that can be used to add providers only to the root module in accordance with the Angular docs. So my proposal is to remove services that are meant to be used as a singleton by client modules from the providers arrays of material modules. Change

@NgModule({
  providers:    [ MdIconRegistry]
})
export class MdIconModule{ }

to

@NgModule({
  providers: [] //empty since we want a singleton service
})
export class MdIconModule{
  static forRoot() {
    return {ngModule: MdIconModule, providers: [ MdIconRegistry]};
  }
}

So that in the root AppModule we can import MdIconModule.forRoot(), thus registering its provider just once to all components including lazy-loaded ones.

What is the current behavior?

Because even services that should be singletons are included in the providers array of their respective Material modules, I don't know how it's possible to make them singletons. To pick up the example above, when I execute MdIconRegistry.addSvgIconSet() in my AppComponent, the icons are not visible to lazy-loaded modules because they have a different Angular services injector.

What are the steps to reproduce?

Open this plunker and navigate to the LazyLoadedComponent view. You'll notice that the icon doesn't show up and the console logs the error below:

Error retrieving icon: Error: Unable to find icon with the name ":play"

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

The current setup makes services that are only practical as singletons really difficult to use. The only workarounds I've found are:

  • Load the whole app at bootstrap (no lazy-loading): this obviously has very negative performance repercussions
  • Register the same icon set in every component: this triggers a new http request for the same file every time the user navigates, not only harming client performance but also taxing the server

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

Angular 2 rc.5 with Material 2 alpha 7-3

Is there anything else we should know?

I've been studying this issue for a while now and this post is the culmination of my observations in #1022 and #1016 . The Angular docs recommend a strategy akin to what I propose here:

forRoot and forChild are conventional names for methods that deliver different import values to root and feature modules. Follow the convention when you write similar modules for your application.

@jelbourn jelbourn changed the title [important] Make Material modules compatible with lazy-loaded components Make Material modules compatible with lazy-loaded components Aug 18, 2016
@jelbourn jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Aug 18, 2016
@jelbourn jelbourn added this to the alpha.8 milestone Aug 18, 2016
@jelbourn jelbourn self-assigned this Aug 18, 2016
@RoxKilly
Copy link
Author

RoxKilly commented Aug 18, 2016

A user with the handle 'James' on StackOverflow helped me find an acceptable workaround until lazy routes are supported.

Basically, instead of importing MdIconModule, build a custom module (I call it MdIconFixedModule) that declares and exports MdIcon, but that has an empty providers array:

@NgModule({
    imports: [CommonModule, HttpModule],
    declarations: [MdIcon],
    exports: [MdIcon],
    providers: [],//leave empty to avoid multiple instances of MdIconRegistry
})
export class MdIconFixedModule {
    static forRoot() {
        return {
            ngModule: MdIconFixedModule,
            providers: [MdIconRegistry] //will be available only to whoever calls .forRoot()
        };
    }
}

Import MdIconFixedModule.forRoot() into AppModule. This provides both MdIcon and a singleton MdIconRegistry. To use icons in other modules, import MdIconFixedModule. This makes MdIcon available without a separate instance of MdIconRegistry, thus fixing the problem.

See this workaround on plunker

@jelbourn
Copy link
Member

Should be fixed with #1108

@RoxKilly
Copy link
Author

@jelbourn There is a typo in the Readme at 91a7e5e#diff-e342377e67af0990cf438fc364e6308dR14

You're importing MdLisTModule instead of MdListModule

RoxKilly pushed a commit to RoxKilly/angular2-seed-material2 that referenced this issue Oct 24, 2016
forRoot() should only be used when importing into a root module. Using forRoot() here will cause Material services that only work as singletons (eg: MdIconRegistry) to fail. See angular/components#1071
RoxKilly pushed a commit to RoxKilly/angular2-seed-material2 that referenced this issue Oct 24, 2016
This must be done at the root module so Material services can work as singletons even with lazy-loaded components. See angular/components#1071
@medeirosrich
Copy link

@RoxKilly Does this still require a workaround? @jelbourn mentioned it should be fixed, Could someone please provide a working example if there is a proper way to do this?

@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful
Projects
None yet
Development

No branches or pull requests

3 participants