10000 ngOnDestroy not fired when service is provided using useFactory · Issue #14818 · angular/angular · GitHub
[go: up one dir, main page]
Skip to content

ngOnDestroy not fired when service is provided using useFactory #14818

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.

Sign up for GitHub

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

Jump to bottom
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
Labels
area: core Issues related to the framework runtime freq1: low type: bug/fix
Milestone
Backlog

Comments

@colinkahn
Copy link
colinkahn commented Feb 28, 2017 •
edited
Loading

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

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

@colinkahn
Copy link
Author

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.
8000

@colinkahn
Copy link
Author

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,
]

@IgorMinar IgorMinar added area: core Issues related to the framework runtime type: bug/fix labels Mar 13, 2017
@ngbot ngbot bot added this to the Backlog milestone Jan 23, 2018
@trotyl trotyl mentioned this issue Feb 18, 2018
Closed
@trotyl trotyl mentioned this issue Feb 27, 2018
Closed
@mhevery mhevery mentioned this issue May 7, 2018
Closed
alxhub added a commit to alxhub/angular that referenced this issue May 7, 2018
@alxhub
fix(core): call ngOnDestroy on all services that have it
97b3aa6 
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 pushed a commit that referenced this issue May 8, 2018
@alxhub @IgorMinar
fix(core): call ngOnDestroy on all services that have it (#23755)
5581e97 
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
@trotyl trotyl mentioned this issue Aug 4, 2018
Closed
@SonyStone
Copy link
SonyStone commented Aug 25, 2018 •
edited
Loading

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 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.

@simeyla simeyla mentioned this issue Aug 29, 2019
Closed
@lloydaf
Copy link
lloydaf commented Aug 29, 2019

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

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

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.
Assignees
No one assigned
Labels
area: core Issues related to the framework runtime freq1: low type: bug/fix
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@IgorMinar @chuckjaz @colinkahn @simeyla @lloydaf @SonyStone @DzmitryShylovich
0