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

ngOnDestroy not called for injectables created via factory providers #22466

Closed
IgorMinar opened this issue Feb 27, 2018 · 9 comments
Closed

ngOnDestroy not called for injectables created via factory providers #22466

IgorMinar opened this issue Feb 27, 2018 · 9 comments

Comments

@IgorMinar
Copy link
Member

@IgorMinar IgorMinar commented Feb 27, 2018

Current behavior

export class DestroyTest implements OnDestroy {
  ngOnDestroy() {  // <<<<< this method is NEVER called
    console.log('ngOnDestroy DestroyTest called');
  }
}

export class DestroyTest2 implements OnDestroy {
  ngOnDestroy() { // <<<<< this method IS called when the app ref is being destroyed
    console.log('ngOnDestroy DestroyTest2 called');
  }
}

export function createDestroyTest() {
  return new DestroyTest();
}

@NgModule({
  imports:      [ BrowserModule, FormsModule ],
  declarations: [ AppComponent, HelloComponent ],
  bootstrap:    [ AppComponent ],
  providers: [
    {provide: DestroyTest, useFactory: createDestroyTest},
    DestroyTest2
  ]
})
export class AppModule {
  constructor(
    destroyTest1: DestroyTest, destroyTest2: DestroyTest2) {}
}

Expected behavior

the ngOnDestroy method, if present, should be called on all values provided via DI regardless of the provider type.

Minimal reproduction of the problem with instructions

https://stackblitz.com/edit/angular-h15slp

Environment


Angular version: 6.0.0-beta.x

// Note: reported by GX

@trotyl
Copy link
Contributor

@trotyl trotyl commented Feb 27, 2018

Duplicate of #14818, #22240

@tmair
Copy link

@tmair tmair commented Mar 1, 2018

There are already bugs within the angular framework, that are caused by this issue (for example the Router is created via a factory function).

The associated bugs contain some comments, that it is currently not possible to do an analysis at compile time if the factory function returns an object that implements the ngOnDestroy hook and that currently this works as intended.

@ngbot ngbot bot removed this from the needsTriage milestone Mar 2, 2018
@ngbot ngbot bot added this to the Backlog milestone Mar 2, 2018
@swicken
Copy link

@swicken swicken commented Mar 22, 2018

We are encountering what appears to be this issue in our application as well. Any route changes with routerLink or router.navigate, ngOnDestroy is not called and the component becomes detached in memory. This is causing a pretty severe memory leak.

Does anyone have any workarounds for this?

@alxhub alxhub self-assigned this Mar 22, 2018
alxhub added a commit to alxhub/angular that referenced this issue May 7, 2018
Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes angular#22466
alxhub added a commit to alxhub/angular that referenced this issue May 7, 2018
Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes angular#22466
alxhub added a commit to alxhub/angular that referenced this issue May 7, 2018
Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes angular#22466
alxhub added a commit to alxhub/angular that referenced this issue May 7, 2018
Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes angular#22466
Fixes angular#22240
Fixes angular#14818
alxhub added a commit to alxhub/angular that referenced this issue May 7, 2018
Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes angular#22466
Fixes angular#22240
Fixes angular#14818
@IgorMinar IgorMinar closed this in fc03427 May 8, 2018
IgorMinar added a commit that referenced this issue May 8, 2018
Previously, ngOnDestroy was only called on services which were statically
determined to have ngOnDestroy methods. In some cases, such as with services
instantiated via factory functions, it's not statically known that the service
has an ngOnDestroy method.

This commit changes the runtime to look for ngOnDestroy when instantiating
all DI tokens, and to call the method if it's present.

Fixes #22466
Fixes #22240
Fixes #14818

PR Close #23755
@amitport
Copy link
Contributor

@amitport amitport commented Jul 4, 2018

@IgorMinar @alxhub this is not resolved for me after fc03427
(using angular 6.1.0-beta.3)

@SonyStone
Copy link

@SonyStone SonyStone commented Aug 25, 2018

I have same problem with Factory providers from this gide:

https://angular.io/guide/dependency-injection#factory-providers

Minimal reproduction of the problem with instructions
https://stackblitz.com/edit/angular-service-factory-issue?file=src%2Fapp%2Ftest.service.ts

Click the "toggle" button one or many times
Test 1 Service will be created and destroyed. Test 2 Service will be created and never destroyed.

Environment
Angular version: 6.1.0

@llorenspujol
Copy link

@llorenspujol llorenspujol commented Feb 14, 2019

Still happening on angular version 8.0.0-beta.3.

This MR fc03427 solved the issue for the services provided at Module level, not at Component level.

Is there something that privates to solve the issue also for services provided at Component level?
it is quite annoying that in some cases it works and in others it does not 😕 .

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Feb 14, 2019

Is there something that privates to solve the issue also for services provided at Component level?
it is quite annoying that in some cases it works and in others it does not

@llorenspujol could you please create a new issue with a minimal reproduce scenario in a Stackblitz? Module and component level providers go through a slightly different path so we might have missed it.

@llorenspujol
Copy link

@llorenspujol llorenspujol commented Feb 14, 2019

@pkozlowski-opensource I have created an issue explaining the bug: #28738. I also have created a MR: #28737 that includes 2 test that covers the destroy functionality of the services provided at component level. One pass and one fails, and they both should pass.

@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Sep 14, 2019

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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

10 participants