angular / angular

Deliver web apps with confidence 🚀
https://angular.dev
MIT License
95.71k stars 25.26k forks source link

Dependency injection creates new instance each time if used in router submodule #12889

Closed peku33 closed 7 years ago

peku33 commented 7 years ago

I'm submitting a ... (check one with "x")

[X] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior My application is divided into modules. Router navigates between modules, each child modules has some providers and some components. I expect application to reuse services injected to components within one module. However, when I navigate, service constructor is called each time component is loaded.

RootModule:

        RouterModule.forRoot([
            { path: '', component: EmptyComponent },
            { path: 'module', loadChildren: () => SubModule },
        ]) 

SubModule:

        RouterModule.forChild([
            { path: 'test1', component: TestComponent1 },
            { path: 'test2', component: TestComponent2 },
        ])

...
    providers:
    [
        TestService
    ],

Each time I navigate between /module/test1 and /module/test2 - TestService constructor is called

Expected behavior If test1 and test2 are defined as routes in RootModule, only one instance of TestService is created. I expect SubModule to inject the same instance of TestService if I navigate between its components.

If I move TestService provider to RootModule - works as expected

Minimal reproduction of the problem with instructions https://github.com/peku33/angular2-dependency-injection-failure

What is the motivation / use case for changing the behavior? Most of components of one of modules uses a service that opens and maintains WebSocket connection. Each time a service is spawned, new connection is opened. I definitely want to reuse the component and keep single connection open than close and reopen it every time user navigates between pages.

Please tell us about your environment: Everything is up to date, browser - Chrome

slubowsky commented 7 years ago

Sounds like https://github.com/angular/angular/issues/12869

peku33 commented 7 years ago

I don't think its the same. In my case object is created each time I navigate.

guojenman commented 7 years ago

@peku33 loadChildren creates a new DI context for services. If you want to share services across modules you'll need to put those into a shared module and import them into the root module using the forRoot static method.

new: shared.module.ts

import { NgModule, ModuleWithProviders } from '@angular/core';
import { TestService } from './test.service'

@NgModule({})
export class SharedModule {
  static forRoot(): ModuleWithProviders {
    return {
      ngModule: SharedModule,
      providers: [TestService]
    };
  }
}

root.module.ts

imports: [
        BrowserModule,
        RouterModule.forRoot([
            { path: '', component: EmptyComponent },
            { path: 'module', loadChildren: () => SubModule },
        ]),
        SharedModule.forRoot()
    ],
peku33 commented 7 years ago

@guojenman I know. But the problem is, that DI creates multiple objects for components inside THE SAME module

maartenvana commented 7 years ago

We have the same issue at this moment.

I've edited the plunker from the module implementation example, the constructor of the crisis service now display's an alert when constructed: https://plnkr.co/edit/6yZU3bRtGeXu9Gm1oVrN?p=preview

samalexander commented 7 years ago

We are also seeing the same issue when using services for inter-component communication in lazy-loaded modules. It is forcing us to stick with version 2.1.2 for the time being. I have checked with version 2.2.1 (released today) and can confirm that the issue still exists.

toedter commented 7 years ago

I have a similar issue at the moment (using angular 2.2.1). While my services (all declared as providers in the root module) are instantiated only once , all of my components a newly created when I route to them.

DanielMcAssey commented 7 years ago

I have the exact same issue my services keep getting recreated in the components even though the only time the service is in the providers array is in the root sub-module. Running 2.2.1

httpdigest commented 7 years ago

Same problem here. Everything worked flawlessly with Angular 2.1.2 and Router 3.1.2. After upgrading to Angular 2.2.3 and Router 3.2.3 -> boom! When I added a constructor() to the module, I could confirm that even the whole module was instantiated multiple times, and not just the provider declared within that module. The module was activated lazily with a:

{
    path: 'somepath',
    loadChildren: 'app/somepath/somepath.module#SomePathModule'
}

inside an app.routing.ts, which is included in an RouterModule.forRoot(appRoutes) which itself is added to the imports statement of the app.module.ts's NgModule.

cheng1994 commented 7 years ago

Can also verify that 2.1.2 removes the issue of services within a module being instantiated every time a route changes. 2.2.x and 2.3.0 both have this issue so it is not resolved yet.

Shananra commented 7 years ago

I am experiencing this issue on 4.2.4.

jasminVC commented 7 years ago

Hello, I am experiencing the same issue. I am using, @angular/cli: 1.2.4 node: 8.1.3 os: win32 x64 @angular/animations: 4.0.1 @angular/common: 4.0.1 @angular/compiler: 4.0.1 @angular/core: 4.0.1 @angular/forms: 4.0.1 @angular/http: 4.0.1 @angular/platform-browser: 4.0.1 @angular/platform-browser-dynamic: 4.0.1 @angular/router: 4.0.1 @angular/cli: 1.2.4 @angular/compiler-cli: 4.0.1

does anyone have any option/way to solve this?

0x-2a commented 7 years ago

For those still experiencing this issue after it has been fixed:

Watch out for side-effects of route configuration that may make it seem like a new service instance is being injected for different routes. For example, Desktop Chrome and Mobile Safari have different outcomes for the following:

Template:

<form (ngSubmit)="save()" >
<button class="btn btn-default"
    (click)="save()">Save</button>
</form>

Component:

save() {
    //Do stuff
    this._router.navigate(['/users', { id: userId } ]);
}

In mobile safari, a hard reload happens on the save router-navigate (because the button type defaults to 'submit'). In Desktop Chrome it operates as a refreshless hash change. A hard reload obviously creates a new service instance.

https://stackoverflow.com/a/38144812/786389

Keksdroid commented 6 years ago

I'm experiencing this issue as well. Is it still a present bug or is the problem on my end?

Angular 5.1.3

Dethlore commented 6 years ago

I'm experiencing this issue as well on Angular 5

jbgraug commented 6 years ago

Same here, although the configuration is a bit different. What i'm trying to achieve here is to always render ParentComponent and Lazy-One Module or Lazy2 Module depending on the url.

The ParentComponent should be the same instance but i gets re-instantiated upon navigation between "/" and "two" and vice-versa.

const routes: Routes = [
  {
    path: '',
    component: ParentComponent,
    pathMatch: 'full',
    loadChildren: 'app/LazyModuleTwo/lazy-one.module#OneModule'
  },
  {
    path: 'two',
    component: ParentComponent,
    pathMatch: 'full',
    loadChildren: 'app/LazyModuleTwo/lazy-two.module#TwoModule'
  },
];
mlc-mlapis commented 6 years ago

@jbgraug ... just a question ... where did you get your routes syntax above ... especially mentioning component: ParentComponent?

jbgraug commented 6 years ago

It is just an example, my application is more complicated than that. ParentComponent is a part of another module which is lazyLoaded too (not OneModule or TwoModule).

import { ParentComponent } from './components/parent.component';

const routes: Routes = [
  {
    path: '',
    component: ParentComponent,
    pathMatch: 'full',
    loadChildren: 'app/LazyModuleTwo/lazy-one.module#OneModule'
  },
  {
    path: 'two',
    component: ParentComponent,
    pathMatch: 'full',
    loadChildren: 'app/LazyModuleTwo/lazy-two.module#TwoModule'
  },
];

export const routes = RouterModule.forChild(routes);

@mlc-mlapis Not sure if i understood your question...

By the way i'm using Angular ^5.2.0

mlc-mlapis commented 6 years ago

@jbgraug ... because it should be something like ... this level could be a child for any parent of course ...

{
   path: 'one',
   loadChildren: 'app/LazyModuleTwo/lazy-one.module#OneModule'
},

where routing for OneModule ...

{
   path: '',
   component: ...' // A component from `OneModule`
},
...
jbgraug commented 6 years ago

@mlc-mlapis , It is not the same. I need the ParentComponent to be rendered along with the Childs.

Think of some Master/detail where navigation should render one detail module or the other (lazily) The Master module is i lazy loaded itself, so the behaviour as is, is the correct one. The problem is that the parent component is recreated every time along with the other lazy modules

mlc-mlapis commented 6 years ago

@jbgraug ... still not understand to which <router-outlet> ... and what component should be displayed from a lazy module when in the same time you want to display ParentComponent in the same place.

Or you mean that ParentComponent has its own <router-outlet> to which a component from a lazy loaded module is placed?

BTW ... path: '' and path: 'two' represent two different routes even mapped to the same component. And the only case where an component instance is kept without recreating is the syntax /xxx/:id. In any other case a new component instance is created when you route from one to the other route.

jbgraug commented 6 years ago

Or you mean that ParentComponent has its own to which a component from a lazy loaded module is placed?

BTW ... path: '' and path: 'two' represent two different routes even mapped to the same component. And the only case where an component instance is kept without recreating is the syntax /xxx/:id. In any other case a new component instance is created when you route from one to the other route.

Thats exactly the case. I'd need to achieve the same without using :id ad recreating the component. That's why i guess it is a bug (there is no flag to allow this behaviour).

mlc-mlapis commented 6 years ago

@jbgraug ... hmm, so you are describing a new functionality ... and then it is necessary to ...

  1. Record it as a new feature request.
  2. Describe the logic and syntax in details, including real cases which show that it is really necessary to introduce this new feature (any additional code increases the size).
  3. Remember that it is necessary to not introduce any breaking change so the new syntax has to have no effect on the actual API.
jbgraug commented 6 years ago

@mlc-mlapis I've achieved the desired behaviour (only one parent instance) by changing the structure to this:

import { ParentComponent } from './components/parent.component';

const routes: Routes = [
  {
    path: '',
    component: ParentComponent,
    children: [
         {
            path: '',
            loadChildren: 'app/LazyModuleOne/lazy-one.module#OneModule'
         },
         {
            path: 'two',
            loadChildren: 'app/LazyModuleTwo/lazy-two.module#TwoModule'
         }
     ]
   }
];

export const routes = RouterModule.forChild(routes);

I guess the doc could be improved in https://angular.io/guide/router Thanks for your help.

kresli commented 6 years ago

Why is this closed? https://stackblitz.com/edit/angular-ntya7c?file=src/app/customers/customers.module.ts In the example, each lazy loaded route creating its own instance. I would expect the service is shared between routes. Am I missing something?

sijucm commented 6 years ago

Same issue here. I just modified the angular documentation example on singleton by just adding an alert: https://stackblitz.com/edit/angular-xqg8e9

Click on the Customer route to see the constructor being called.

sunilpes commented 6 years ago

@sijucm I'm facing exactly same issue for Lazy loaded modules. Shared providers are instantiated twice though we have followed 'forRoot' convention at the root module.

Folks, Any updates on this issue?

This issue still exist in 5.2.11 version

I really appreciate a quick response from the team.

sunilpes commented 6 years ago

workaround for this issue:

@Injectable()
export class ServiceA {
      static singletonInstance: ServiceA;

      constructor() {
         if (!ServiceA.singletonInstance) {
           // construct object
           ServiceA.singletonInstance = this;
          }
          return ServiceA.singleInstance
       }

}

I reallly hate this to do in the project :( but no chance

angular-automatic-lock-bot[bot] commented 5 years ago

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.