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 fired when service is provided using useFactory #14818

Closed
colinkahn opened this issue Feb 28, 2017 · 8 comments
Closed

ngOnDestroy not fired when service is provided using useFactory #14818

colinkahn opened this issue Feb 28, 2017 · 8 comments

Comments

@colinkahn
Copy link

@colinkahn colinkahn commented Feb 28, 2017

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

[ ] bug report => search github for a similar issue or PR before submitting
[x] 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
When you provide a service using useFactory, ngOnDestroy will never be called.

Expected behavior
Regardless of how you provide the service, ngOnDestroy should get called if it is a method on the service.

Minimal reproduction of the problem with instructions
https://plnkr.co/edit/aUaz6ewyM3m5JorsAoCO?p=preview

  1. Open the console.
  2. Click the "Create Component" button one or many times
  3. After a 2 second delay the component will be destroyed, but no console output will be seen.
  4. If you replace useFactory with useClass the problem goes away and you'll see output.

What is the motivation / use case for changing the behavior?
There are many cases, main motivator is that this isn't intuitive.

Please tell us about your environment:

  • Angular version: 2.4.8
  • Browser: Chrome 56.0.2924.87
  • Language: TypeScript 2.0

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

@DzmitryShylovich
Copy link
Contributor

@DzmitryShylovich DzmitryShylovich commented Feb 28, 2017

it's by design. there's no way to know if returned by the factory object has ngOnDestroy or not.

@colinkahn
Copy link
Author

@colinkahn colinkahn commented Feb 28, 2017

As a user useClass, useValue, and useFactory would seem to all just be different ways to achieve the same result. I think this should be a feature if it's not possible in the current implementation.

@colinkahn
Copy link
Author

@colinkahn colinkahn commented Feb 28, 2017

A few other related examples showing that even useClass doesn't always work:

https://plnkr.co/edit/eIs7pjvldhcFrMmaJ4IV?p=preview

In that version I'm providing an abstract class and using useClass to specify a concrete version. ngOnDestroy is never called.

https://plnkr.co/edit/G7vLIvA1nz1dBoJ4sPcW?p=preview

Similar to the above, but using an OpaqueToken.

As far as I can tell the only time ngOnDestroy does work is if you use one of these (I believe they're equivalent):

[
  {
    provide: X,
    useClass: X,
  },
  // OR
  X,
]
@ngbot ngbot bot added this to the Backlog milestone Jan 23, 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
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
@SonyStone
Copy link

@SonyStone SonyStone commented Aug 25, 2018

I have simuler issue 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.
@simeyla
Copy link

@simeyla simeyla commented Aug 29, 2019

I can't believe it doesn't work with an InjectionToken (used to be OpaqueToken).
That was very unexpected!

I need to unsubscribe to some observables in my service - I ended up having to inject a 'SubscriptionManager' service to my component and had the dependent service use that.

I suppose in future maybe it's better to have the component call init on the service (to return Subscription instance) and then track the subscriptions in the component.

@lloydaf
Copy link

@lloydaf lloydaf commented Aug 29, 2019

@simeyla do you have any gists to demonstrate using a subscription manager?

@simeyla
Copy link

@simeyla simeyla commented Aug 29, 2019

@lloydaf I'm not particularly happy with the way mine works - the boiler plate to just use it is equal to the effort to just manually create a Subscription.

The goal was to just make an Injectable that a component would provide to itself and you would 'add' subscriptions to it. First problem is that you can't disable inheritance for DI objects - so if you forget to provide it then you'll get the parent manager. So I added a mechanism to make sure it was initialized only once - but then you have to call init() in the constructor which is just ugly. It also put all the subscriptions in a global manager which in the end didn't really bring me the other benefits I was hoping for (such as counting subscriptions and reporting if you weren't closing them properly).

So tbh my 'subscription manager' ended up kind of a mess and I'm phasing it out where I did use it. A simple 'Subscription' object on the class or using something like takeUntilDestroy works better. Also there are more issues with it that are even more complicated and 'providing' it to you would be more of a foot gun than anything else.

If you're only using ngOnDestroy to unsubscribe to observables you can do one of two things:

  1. Only allow the component to own the Subscription - eg. coding standard says services can't subscribe.

  2. I just came up with this and I actually kind of like it for my situation. Simple and clear and relatively safe.

export const COMPONENT_SUBSCRIPTION = new InjectionToken<Subscription>("COMPONENT_SUBSCRIPTION");

Then a factory function to create it:

export const componentSubscriptionFactory = () => new Subscription();

In every component you must provide it - and be sure to use @self to prevent inheriting a parent subscription (big mess if you don't):

        {
            provide: COMPONENT_SUBSCRIPTION,
            useFactory: componentSubscriptionFactory
        }

@Self() @Inject(COMPONENT_SUBSCRIPTION) private subscription: Subscription,

You must also put this in the component (only)

ngOnDestroy() 
{
    debugger;  // just to prove it works!
    this.subscription.unsubscribe();
}

and in services you use this:

@Inject(COMPONENT_SUBSCRIPTION) subscription: Subscription

The service will then get your component's Subscription object and it's just a normal RxJS object you can call add(...) on.

And none of this is any help if you need to dispose of anything other than RxJS subscriptions!

@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Sep 29, 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 29, 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.

None yet
7 participants
You can’t perform that action at this time.