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

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

Closed
peku33 opened this issue Nov 15, 2016 · 29 comments
Closed
Labels
area: core Issues related to the framework runtime area: router type: bug/fix

Comments

@peku33
Copy link

peku33 commented Nov 15, 2016

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

  • Angular version: 2.0.X
    Have 'latest' for almost everything in package.json
    @angular/core@2.2.0

  • Browser: [all | Chrome XX | Firefox XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

  • Language: [all | TypeScript X.X | ES6/7 | ES5]

  • Node (for AoT issues): node --version =

@slubowsky
Copy link

Sounds like #12869

@peku33
Copy link
Author

peku33 commented Nov 15, 2016

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

@guojenman
Copy link

@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
Copy link
Author

peku33 commented Nov 15, 2016

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

@chuckjaz chuckjaz added the area: core Issues related to the framework runtime label Nov 16, 2016
@maartenvana
Copy link

maartenvana commented Nov 17, 2016

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

  • Navigate to crisis center -> alert pops up
  • Click on an item in the list
  • Click on crisis list at the bottom -> alert pops up

@samalexander
Copy link

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
Copy link

toedter commented Nov 20, 2016

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
Copy link

DanielMcAssey commented Nov 23, 2016

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
Copy link

httpdigest commented Nov 25, 2016

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
Copy link

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.

DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Jan 31, 2017
DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Feb 4, 2017
vicb added a commit to vicb/angular that referenced this issue Mar 10, 2017
fixes angular#12869
fixes angular#12889
fixes angular#13885
fixes angular#13870

Before this change there was a single injector tree.
Now we have 2 injector trees, one for the modules and one for the components.
This fixes lazy loading modules.

See the design docs for details:
https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4

BREAKING CHANGES

`ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter.
No change should be required in user code as the correct module will be used
when none is provided

DEPRECATIONS

The following methods were used internally and are no more required:
- `RouterOutlet.locationFactoryResolver`
- `RouterOutlet.locationInjector`
vicb added a commit to vicb/angular that referenced this issue Mar 10, 2017
fixes angular#12869
fixes angular#12889
fixes angular#13885
fixes angular#13870

Before this change there was a single injector tree.
Now we have 2 injector trees, one for the modules and one for the components.
This fixes lazy loading modules.

See the design docs for details:
https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4

BREAKING CHANGES

`ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter.
No change should be required in user code as the correct module will be used
when none is provided

DEPRECATIONS

The following methods were used internally and are no more required:
- `RouterOutlet.locationFactoryResolver`
- `RouterOutlet.locationInjector`
vicb added a commit to vicb/angular that referenced this issue Mar 10, 2017
fixes angular#12869
fixes angular#12889
fixes angular#13885
fixes angular#13870

Before this change there was a single injector tree.
Now we have 2 injector trees, one for the modules and one for the components.
This fixes lazy loading modules.

See the design docs for details:
https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4

BREAKING CHANGES

`ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter.
No change should be required in user code as the correct module will be used
when none is provided

DEPRECATIONS

The following methods were used internally and are no more required:
- `RouterOutlet.locationFactoryResolver`
- `RouterOutlet.locationInjector`
vicb added a commit to vicb/angular that referenced this issue Mar 13, 2017
fixes angular#12869
fixes angular#12889
fixes angular#13885
fixes angular#13870

Before this change there was a single injector tree.
Now we have 2 injector trees, one for the modules and one for the components.
This fixes lazy loading modules.

See the design docs for details:
https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4

BREAKING CHANGES

`ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter.
No change should be required in user code as the correct module will be used
when none is provided

DEPRECATIONS

The following methods were used internally and are no more required:
- `RouterOutlet.locationFactoryResolver`
- `RouterOutlet.locationInjector`
SamVerschueren pushed a commit to SamVerschueren/angular that referenced this issue Mar 18, 2017
fixes angular#12869
fixes angular#12889
fixes angular#13885
fixes angular#13870

Before this change there was a single injector tree.
Now we have 2 injector trees, one for the modules and one for the components.
This fixes lazy loading modules.

See the design docs for details:
https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4

BREAKING CHANGES

`ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter.
No change should be required in user code as the correct module will be used
when none is provided

DEPRECATIONS

The following methods were used internally and are no more required:
- `RouterOutlet.locationFactoryResolver`
- `RouterOutlet.locationInjector`
smurfy pushed a commit to smurfy/angular that referenced this issue Apr 7, 2017
fixes angular#12869
fixes angular#12889
fixes angular#13885
fixes angular#13870

Before this change there was a single injector tree.
Now we have 2 injector trees, one for the modules and one for the components.
This fixes lazy loading modules.

See the design docs for details:
https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4

BREAKING CHANGES

`ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter.
No change should be required in user code as the correct module will be used
when none is provided

DEPRECATIONS

The following methods were used internally and are no more required:
- `RouterOutlet.locationFactoryResolver`
- `RouterOutlet.locationInjector`
@Shananra
Copy link

Shananra commented Jul 3, 2017

I am experiencing this issue on 4.2.4.

@jasminVC
Copy link

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?

asnowwolf pushed a commit to asnowwolf/angular that referenced this issue Aug 11, 2017
fixes angular#12869
fixes angular#12889
fixes angular#13885
fixes angular#13870

Before this change there was a single injector tree.
Now we have 2 injector trees, one for the modules and one for the components.
This fixes lazy loading modules.

See the design docs for details:
https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4

BREAKING CHANGES

`ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter.
No change should be required in user code as the correct module will be used
when none is provided

DEPRECATIONS

The following methods were used internally and are no more required:
- `RouterOutlet.locationFactoryResolver`
- `RouterOutlet.locationInjector`
juleskremer pushed a commit to juleskremer/angular that referenced this issue Aug 28, 2017
fixes angular#12869
fixes angular#12889
fixes angular#13885
fixes angular#13870

Before this change there was a single injector tree.
Now we have 2 injector trees, one for the modules and one for the components.
This fixes lazy loading modules.

See the design docs for details:
https://docs.google.com/document/d/1OEUIwc-s69l1o97K0wBd_-Lth5BBxir1KuCRWklTlI4

BREAKING CHANGES

`ComponentFactory.create()` takes an extra optional `NgModuleRef` parameter.
No change should be required in user code as the correct module will be used
when none is provided

DEPRECATIONS

The following methods were used internally and are no more required:
- `RouterOutlet.locationFactoryResolver`
- `RouterOutlet.locationInjector`
@0x-2a
Copy link

0x-2a commented Sep 12, 2017

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
Copy link

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
Copy link

Dethlore commented Feb 7, 2018

I'm experiencing this issue as well on Angular 5

@jbgraug
Copy link

jbgraug commented Feb 16, 2018

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.

  • For the url "/" lazy-one.module should be rendered.
  • For the url "/two" lazy-two.module should be rendered.

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
Copy link
Contributor

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

@jbgraug
Copy link

jbgraug commented Feb 16, 2018

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
Copy link
Contributor

mlc-mlapis commented Feb 16, 2018

@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
Copy link

jbgraug commented Feb 16, 2018

@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
Copy link
Contributor

@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
Copy link

jbgraug commented Feb 16, 2018

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
Copy link
Contributor

mlc-mlapis commented Feb 16, 2018

@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
Copy link

jbgraug commented Feb 16, 2018

@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
Copy link

kresli commented Mar 1, 2018

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
Copy link

sijucm commented Mar 1, 2018

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
Copy link

sunilpes commented Jun 8, 2018

@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
Copy link

sunilpes commented Jun 8, 2018

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
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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime area: router type: bug/fix
Projects
None yet
Development

No branches or pull requests