Unable to overwrite ngDestroy inside the component

🐞 bug report

Affected Package

The issue is caused by package @angular/....

Is this a regression?

Yes, the previous version in which this bug was not present was: 8.2

Description

Please take a look here for a source code:
https://github.com/xtephan/ng-destroy-test/blob/master/src/app/destroyable/destroyable.component.ts

When the DestroyableComponent is being destroyed, the ngOnDestroy method as defined on line 16 is called, instead of as defined on line 11.

If inspecting the component using the devtools, ngOnDestroy method points to being located on line 11:

🔬 Minimal Reproduction

https://github.com/xtephan/ng-destroy-test

The master branch contains ng9, where bug can be seen present.

The ng8 branch contains ng8, where the bug was not present.

Run the app with ng serve and access http://localhost:4200. In the webpage, press the toggle button to destroy the inner component.

The expected result would to see this.ngOnDestroy called outputed in the console.
Actual behaviour is that DestroyableComponent.ngOnDestroy called is being outputed.

🔥 Exception or Error

None

🌍 Your Environment

Angular Version:


     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/
    

Angular CLI: 9.1.3
Node: 12.4.0
OS: darwin x64

Angular: 9.1.3
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router
Ivy Workspace: Yes

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.901.3
@angular-devkit/build-angular     0.901.3
@angular-devkit/build-optimizer   0.901.3
@angular-devkit/build-webpack     0.901.3
@angular-devkit/core              9.1.3
@angular-devkit/schematics        9.1.3
@ngtools/webpack                  9.1.3
@schematics/angular               9.1.3
@schematics/update                0.901.3
rxjs                              6.5.5
typescript                        3.8.3
webpack                           4.42.0

Anything else relevant?

Related to / duplicate of #16478

The long story short is that we take lifecycle methods from the class definition instead of an instance. This is mostly done for the performance reasons and unlikely to change in the future.

Having said this I would love to hear more about your actual, functional use-case. Why would it be beneficial for you to define lifecycle hooks on an instance? Are you writing a custom decorator?

@xtephan

I work with @xtephan, on the same project, so i can attempt to answer that.

We have a custom subsink, that overwrites the ngDestroy method to allow us to cleanup subscriptions when a component is destroyed. The subsink works by assigning to a property directly on the object, and keeping a reference to the old method, so we can call the onDestroy. So in relation to what is said on the linked issues, specifically here: #7270 (comment), the onDestroy method is there in the original class definition, as can also be seen in the sample provided.

Subsink code:

import { Unsubscribable } from 'rxjs';
import { OnDestroy } from '@angular/core';
import { onNgOnDestroy } from './hooks';

export class SubSink {
  private _subs: Unsubscribable[] = [];

  /**
   * @param component - The component to link the unsubscription to
   */
  public constructor(component: OnDestroy) {
    onNgOnDestroy(component, () => this.unsubscribe());
  }

  /**
   * Adds a single subscription to the sink
   * @param subscription
   */
  public set sink(subscription: Unsubscribable) {
    this._subs.push(subscription);
  }

  /**
   * Adds some subscriptions to the sink
   * @param subcriptions
   */
  public add(...subcriptions: Unsubscribable[]) {
    this._subs.push(...subcriptions);
  }

  public unsubscribe() {
    for (const sub of this._subs) {
      sub.unsubscribe();
    }
    this._subs = [];
  }
}

hooks.ts:

import { OnDestroy } from '@angular/core';

export function onNgOnDestroy(destroyable: OnDestroy, cb: () => void) {
  const original = destroyable.ngOnDestroy;
  destroyable.ngOnDestroy = () => {
    cb();
    original.apply(destroyable);
    destroyable.ngOnDestroy = original;
  };
}

The way the hook is implemented, we can layer multiple on destroy hooks, without them getting in the way of each other, since they will all be called.

The subsink is then used in a component like this:

export class MyComponent implements OnInit, OnDestroy {
   private sink = new SubSink(this);

  ngOnInit() {
     const myObs = /* Something that gets an observable */
     this.sink.sink = myObs.subscribe();
  }

  ngOnDestroy() {
   // Can be left empty if no other teardown is required
}
}

An alternative we have discussed internally as a solution for this specific regression, is making the SubSink an injectable service (With a unique instance for each component instance), and then relying on ngOnDestroy being called on the service. This however then requires that we explicitly pass the SubSink in the provider array of every single component we have. To avoid forgetting to do that, we will need to write a custom lint rule to check that. All in all, quite a bit of work to get around Angular not "observing" the normal rules of how "methods" works in javascript, and instead relying on the method only being defined on the prototype.

@pkozlowski-opensource

The long story short is that we take lifecycle methods from the class definition instead of an instance. This is mostly done for the performance reasons and unlikely to change in the future.

@pkozlowski-opensource Though, there is a PR in place (with you as a reviewer) to revert this behavior and allow for this type of meta-programming.

#35464

Any update on the status of this?

(edit: and if I am understanding that pull request, it isn't changing what you stated in the quote above, but just changing the timing at which the "reading off the class definition" occurs).

Have a look at this https://github.com/ngneat/until-destroy/blob/master/README.md

cheers
flash ⚡

@flash-me

@flash-me We have investigated that, however we don't find it satisfactory, since it requires decorates, which means we need to ship a polyfill for decorators, which we really don't want to, if we can avoid it. And i believe the reason until-destroy started doing the decorators, is exactly because of this issue.

What do you mean by shipping a polyfill for decorators?

I just created following typescript file:

function logger() {
  return function (target: any, propertyKey: string, descriptor: PropertyDescriptor) {
    const original = descriptor.value;
    descriptor.value = (...args: any[]) => {
      console.log("decorated logger");
      original.apply(this, args);
    }
  }
}

class Foo {
  @logger()
  fooMethod() {
    console.log("hi");
  }
}

with the following tsconfig.json:

{
  "compilerOptions": {
    "target": "es5",
    "module": "ESNext",
    "experimentalDecorators": true
  }
}

which will creat following javascript:

var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
    else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    return c > 3 && r && Object.defineProperty(target, key, r), r;
};
function logger() {
    return function (target, propertyKey, descriptor) {
        var _this = this;
        var original = descriptor.value;
        descriptor.value = function () {
            var args = [];
            for (var _i = 0; _i < arguments.length; _i++) {
                args[_i] = arguments[_i];
            }
            console.log("decorated logger");
            original.apply(_this, args);
        };
    };
}
var Foo = /** @class */ (function () {
    function Foo() {
    }
    Foo.prototype.fooMethod = function () {
        console.log("hi");
    };
    __decorate([
        logger()
    ], Foo.prototype, "fooMethod", null);
    return Foo;
}());

Which i can put into browser and execute, even though I compiled it with ES5 target.

cheers
flash ⚡

Regarding your sink: You don't need to play around with arrays. Subscription itself can take other Subscriptions. Once you unsubscribe the "outer" Subscription, every "inner" ones will be unsubscribed, too:

import { Subscription, interval, timer } from 'rxjs';


const subs = new Subscription();

subs.add(
  interval(200).subscribe(console.log)
);

subs.add(
  interval(200).subscribe(console.log)
);

// Unsubscribe all after 2 sec
timer(2000).subscribe(() => subs.unsubscribe());

https://stackblitz.com/edit/rxjs-mfgbuu?embed=1&file=index.ts&hideExplorer=1&hideNavigation=1

cheers
flash ⚡

Here is a minimal reproduce scenario in a stackblitz: https://stackblitz.com/edit/angular-wadewj?file=src/app/app.module.ts
More background info in #38241

@flash-me

@flash-me I'm not sure how your example matches the SubSink example, you are being forced to call unsubscribe yourself. Their example automatically unsubscribes from ngOnDestroy? Or are you just suggesting a different approach to implement the SubSink?

@zlepper We do pretty much the same thing, see https://github.com/vmware/vmware-cloud-director-ui-components/blob/master/projects/components/src/common/subscription/subscription-tracker.ts Did you end up finding a solution that doesn't require calling subsink.unsubscribe() from every ngOnDestroy?

@flash-me

@flash-me I'm not sure how your example matches the SubSink example, you are being forced to call unsubscribe yourself. Their example automatically unsubscribes from ngOnDestroy? Or are you just suggesting a different approach to implement the SubSink?

Additionally adding subscriptions to other subscriptions also causes those subscriptions to be cancelled, if the first subscription completes (At least it did last i tried).

@juanmendes We ended up using a component scoped service instead:

@Injectable()
export class SubSinkService implements OnDestroy {
  private _subs: Unsubscribable[] = [];

  /**
   * Adds a single subscription to the sink
   * @param subscription
   */
  public set sink(subscription: Unsubscribable) {
    this._subs.push(subscription);
  }

  /**
   * Adds some subscriptions to the sink
   * @param subcriptions
   */
  public add(...subcriptions: Unsubscribable[]) {
    this._subs.push(...subcriptions);
  }

  public unsubscribe() {
    for (const sub of this._subs) {
      sub.unsubscribe();
    }
    this._subs = [];
  }

  ngOnDestroy(): void {
    this.unsubscribe();
  }
}

Turns out Angular calls the OnDestroy method on services if they are scoped. So we add the service to our component providers if we need it, then dependency inject it. Then we additionally have a ts-lint rule to make sure you actually remember to add it to providers, and doesn't accidently use the parent's subsink. It's a bit more involved, but we still get the behavior we want, even though it's nowhere near as nice as it used to be.

@zlepper

@zlepper If it's not too much to ask 😅 , would you be able to provide us the code for your custom ts lint rule? It does sound like it could be a hard to track bug if you forget to add to service to the providers array

@juanmendes

@juanmendes Here you go: https://gist.github.com/zlepper/df282c06bafe46342d5b43f4e049726a
It needs to be compiled and such from the TS version, but i figure you can handle that part? :)

@zlepper

@zlepper Awesome! Yes, we already have our custom rules that we also need a separate build step to turn them into JS so I just need to add it to our custom rules folder. Thanks again. I will post a link to our project as reference if anyone needs to see the entire pattern.

Closing as the feature request in #38241 addresses the root cause here.

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

Navigation Menu

Repository navigation

Unable to overwrite ngDestroy inside the component #36776

🐞 bug report

Affected Package

Is this a regression?

Description

🔬 Minimal Reproduction

🔥 Exception or Error

🌍 Your Environment

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Unable to overwrite ngDestroy inside the component #36776

Description

🐞 bug report

Affected Package

Is this a regression?

Description

🔬 Minimal Reproduction

🔥 Exception or Error

🌍 Your Environment

Activity

pkozlowski-opensource commented on Apr 23, 2020

zlepper commented on Apr 23, 2020

Achilles1515 commented on Apr 23, 2020

flash-me commented on Apr 24, 2020

zlepper commented on Apr 24, 2020

flash-me commented on Apr 24, 2020

flash-me commented on Apr 24, 2020

pkozlowski-opensource commented on Jul 27, 2020

juanmendes commented on Aug 26, 2020

zlepper commented on Aug 26, 2020

juanmendes commented on Aug 26, 2020

zlepper commented on Aug 26, 2020

juanmendes commented on Aug 26, 2020

atscott commented on Oct 19, 2020

angular-automatic-lock-bot commented on Nov 19, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions