Seems to be breaking my ngOnDestroy implementation in a class I'm extending from

[fxck]

So I have this class I extend most of my "smart" components from.

import {  OnInit, OnDestroy } from '@angular/core';
import { Store, Action } from '@ngrx/store';
import { Subject, merge, Observable } from 'rxjs';
import { takeUntil, filter } from 'rxjs/operators';

export const isOnInit = () => filter((l) => l === 'OnInit');
export const isOnDestroy = () => filter((l) => l === 'OnDestroy');

export class BaseClass implements OnInit, OnDestroy {
  lifecycle$ = new Subject<'OnInit' | 'OnDestroy'>();

  ngOnInit() {
    this.lifecycle$.next('OnInit');
  }

  ngOnDestroy() {
    this.lifecycle$.next('OnDestroy');
    this.lifecycle$.complete();
  }

  dispatchActions$$(store: Store<any>, actions: Observable<Action>[]) {
    merge(...actions)
      .pipe(takeUntil(this.lifecycle$.pipe(isOnDestroy())))
      .subscribe(store);
  }
}

as soon as @ObservableEvent() is used inside a class that extends from this class I don't get the OnDestroy message from lifecycle$

@d3lm

[fxck]

See https://github.com/fxck/template-streams-ondestroy-but for reproduction @d3lm

[fxck]

Ok, I spent few hours trying to debug this.

As soon as your architect.build.options.aot is set to true, this happens.

Doing

  ngOnDestroy() {
    super.ngOnDestroy();
  }

makes it work even with aot. Really do not want to do this.. I would much rather not have it automatically unsubscribed than having to do this.

[fxck]

Upon further testing, nothing inside ObservableEvent is at fault. Even if it was empty inside, it still breaks that ngOnDestroy in aot. So it's whatever happen on build / with transformers that does it.

[fxck]

Yeah, so hasLifecycleHook assumes that ngOnDestroy is on the class itself, doesn't account for the possibility of it being defined in a class the class extends from, thus overriding it.

There is a way, albeit not easy one phenomnomnominal/tsquery#30 to check the parent class.

Also I still haven't figured out what does aot have to do with this.

[d3lm]

OK, thanks for opening the issue. I have indeed not tested class inheritance. I will try to debug this. AOT is indeed problematic here because in AOT it assumes the lifecycle to be there and my guess is that my TS transformer doesn't properly work in the case of inheritance. I'll have a look.

[fxck]

Any luck @d3lm?

[d3lm]

No, haven't had the time yet. If you want you can look into it. PR is more than welcome. I think the transformer has to be modified to also account for inheritance.

[fxck]

I think that this or rather the query it calls needs to do this phenomnomnominal/tsquery#30 but I haven't really figured out how to even debug this.

[fxck]

What's the dev flow like? Do I run test:debug, put some breakpoints in and use remote dev tools?

[d3lm]

So what happens is that in the build process the library looks at every component and checks if you use one of the observable decorators, e.g ObservableEvent. If that's the case it adds the necessary lifecycle hooks in case they don't exist. The problem now is (at least when it comes to this lib), that you define the lifecycles in the base class so the lib sees your component and also sees there is no ngOnDestroy lifecycle defined. As a result, it transforms the class and adds the lifecycle (only for AOT). This will effectively overwrite the lifecycle from your base class.

I think it's quite tricky to solve this because during the transformation you only look at a single file at a time and you cannot really query for things in the base class, unless you read that file from the file system, but I find this not really ideal. From the top of my head I have no better solution.

The dev flow for me is starting yarn test or yarn test:debug, write a failing test and start implementing the feature. When you start the test command in debug mode you can open Chrome Devtools and attach to the debug process or use VS Code for example.

[fxck]

Could it possibly be done using some sort of configuration? User would list which base classes implement ngOnDestroy, use that in the getLifecycleQuery query.

I imagine only few classes in the whole application do that.

[d3lm]

Yea, one possible solution. Or how about we use a naming convention for the base class if you use this library? Not sure what the name could be tho.

[fxck]

yeah I guess, you could use import alias to rename existing classes

[fxck]

but configuration still sounds better.. or make it possible both ways