SuiLocalizationService not working with lazy loading modules

[avaneev95]

Hi @edcarroll, I'm working on a project with a lot of lazy loading modules. It seems that the SuiLocalizationService isn't working with lazy loading modules. If I change the locale in my AppComponent (and I expect that other modules will use this locale) it won't affect lazy loading modules as they are creating their own SuiLocalizationService instance. The plunkr example ilustrates this issue.

Maybe SuiLocalizationModule can be rewrited like this:

@NgModule({
    imports: [CommonModule]
})
export class SuiLocalizationModule {
    public static forRoot():ModuleWithProviders {
        return {
            ngModule: SuiLocalizationModule,
            providers: [
                SuiLocalizationService
            ]
        };
    }
}

So in root module we can import this module as SuiLocalizationModule.forRoot() and there will be only one SuiLocalizationService.

[edcarroll]

Hi @avaneev95, thanks for opening the issue. I've taken some time and learned a bit more about what is causing this and how to mitigate it, and have come up with 2 possible solutions:

1) Add forRoot to all NgModules, and in a non-lazy loaded module if you want to import some functionality you must use SuiXModule.forRoot(). a) This would be a breaking change from the current behavior. b) A first attempt at an implementation is available in #235. c) Usage:

@NgModule({
    imports: [
        SuiSelectModule.forRoot(),
        SuiCheckboxModule.forRoot()
    ],
    ...
})
export class MyModule

@NgModule({
    imports: [
        SuiSelectModule,
        SuiCheckboxModule
    ],
    ...
})
export class MyLazyModule

2) Add forChild to all NgModules, and in a lazy loaded module if you want to import some functionality you must use SuiXModule.forChild(). a) This slots in with the existing behavior, so no major changes to anyone's codebases. c) Usage:

@NgModule({
    imports: [
        SuiSelectModule,
        SuiCheckboxModule
    ],
    ...
})
export class MyModule

@NgModule({
    imports: [
        SuiSelectModule.forChild(),
        SuiCheckboxModule.forChild()
    ],
    ...
})
export class MyLazyModule

For both, I can make the services in question throw errors when they detect there are multiple versions of themselves in the injector cache.

Thoughts on the above? I have limited experience with lazy loaded modules so input would be very helpful 😄

[avaneev95]

Hi, thanks for your reply, I tested #235 and encountered a problem: for example, I want to use a SuiPopupModule in both non-lazy and lazy modules, so in non-lazy module I import a SuiPopupModule as SuiPopupModule.forRoot() and in lazy - as SuiPopupModule. And when I want to got to the lazy module it doesn't work due to an error: "Type SuiTransition is part of the declarations of 2 modules: SuiTransitionModule and SuiTransitionRootModule". It seems this happens due to SuiTransitionRootModule and SuiTransitionModule are loaded in the same time.

I managed to fix this by moving component declarations to another module like this (ex. SuiPopupModule):

@NgModule({
    imports: [
        ...imports,
        SuiTransitionModule,
        SuiUtilityModule
    ],
    declarations,
    entryComponents,
    exports
})
export class SuiPopupMainModule {}

@NgModule({
    imports: [
        ...imports,
        SuiPopupMainModule,
        SuiTransitionModule.forRoot(),
        SuiUtilityModule.forRoot()
    ],
    providers: [
        SuiPopupConfig
    ],
    exports
})
export class SuiPopupRootModule {}

@NgModule({
    imports: [
        ...imports,
        SuiPopupMainModule
    ],
    exports
})
export class SuiPopupModule {
    public static forRoot():ModuleWithProviders {
        return {
            ngModule: SuiPopupRootModule
        };
    }
}

It works and I think it will work with forChild() as well. In addition, I don't quite understand why all modules should have forRoot() method, even simple ones like SuiTransitionModule which doesn't have any providers.

By the way, about forChild vs forRoot: personally I prefer forRoot since it seems to be used more commonly but maybe it's better to avoid more breaking changes :smile: