Basically I'm still not really sure about the cleanup of reactive code in Angular components. Are the following rules correct and sufficient?
My rules for a reactive component:
- on manual x.subscribe():
- if using x.takeUntil(...) or similar construct then no manual cleanup
- if x.complete() is called in the component scope then no manual cleanup
- else manual .unsubscribe() on demand or in ngOnDestroy()
- on template's observable | async:
- no manual cleanup needed plus onPush compatible
- on manual direct creation of Observable like new Subject():
- manual .complete() in ngOnDestroy() else never closes
- on manual creation using other Observables like Observable.merge(...):
- no manual cleanup needed as long as subscription is cleaned up
I'm not really sure about the last bullet point.
Here is a full example of a real component I refactored according to those rules, can someone see whether there is danger for a memory leak? All public $ observables are subscribed via async pipe in the template so I'm sure that the subscriptions are taken care of. The subjects created with new are manually cleaned up so that should be OK. The parts I'm worried about are the Observable.combineLatest(...) observables.
import { ChangeDetectionStrategy, Component, Input, OnDestroy } from '@angular/core';
import { Router } from '@angular/router';
import { DeliveryPointId } from 'app/bso';
import { BookmarkService } from 'app/general';
import { EAccess, EPermission, ReduxGetters, ReduxService } from 'app/redux';
import * as routing from 'app/routing';
import { BehaviorSubject, Observable } from 'app/rx';
@Component({
selector: 'app-last-opened-workitems-widget',
templateUrl: './last-opened-workitems-widget.component.html',
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class LastOpenedWorkitemsWidgetComponent implements OnDestroy {
private readonly showPartners$ = new BehaviorSubject(true);
private readonly showServices$ = new BehaviorSubject(true);
private readonly showTens$ = new BehaviorSubject(true);
private readonly showWorklist$ = new BehaviorSubject(true);
@Input() set showItems(val: boolean) { this.showWorklist$.next(!!val); }
@Input() set showPartners(val: boolean) { this.showPartners$.next(!!val); }
@Input() set showProcesses(val: boolean) { this.showServices$.next(!!val); }
@Input() set showTens(val: boolean) { this.showTens$.next(!!val); }
constructor(
private readonly redux: ReduxService,
private readonly bookmarks: BookmarkService,
private readonly router: Router,
) { }
canPartners$ = this.redux.watch(ReduxGetters.userAccess).map(access => access[EPermission.Partner] >= EAccess.Read);
canServices$ = this.redux.watch(ReduxGetters.userAccess).map(access => access[EPermission.Services] >= EAccess.Read);
canTens$ = this.redux.watch(ReduxGetters.userAccess).map(access => access[EPermission.Tens] >= EAccess.Read);
canWorklist$ = this.redux.watch(ReduxGetters.userAccess).map(access => access[EPermission.Worklist] >= EAccess.Read);
lastItems$ = this.bookmarks.onLastOpenedWorkitems.map(ii => [...ii].reverse());
lastPartners$ = this.bookmarks.onLastOpenedPartners.map(ii => [...ii].reverse());
lastProcesses$ = this.bookmarks.onLastOpenedProcesses.map(ii => [...ii].reverse());
lastTens$ = this.bookmarks.onLastOpenedTens.map(ii => [...ii].reverse());
hasContentPartners$ = Observable
.combineLatest(
this.showPartners$.distinctUntilChanged(),
this.canPartners$,
this.lastPartners$.map(ii => ii.length > 0))
.map(oks => oks.every(ii => ii));
hasContentServices$ = Observable
.combineLatest(
this.showServices$.distinctUntilChanged(),
this.canServices$,
this.lastProcesses$.map(ii => ii.length > 0))
.map(oks => oks.every(ii => ii));
hasContentTens$ = Observable
.combineLatest(
this.showTens$.distinctUntilChanged(),
this.canTens$,
this.lastTens$.map(ii => ii.length > 0))
.map(oks => oks.every(ii => ii));
hasContentWorklist$ = Observable
.combineLatest(
this.showWorklist$.distinctUntilChanged(),
this.canWorklist$,
this.lastItems$.map(ii => ii.length > 0))
.map(oks => oks.every(ii => ii));
hasContent$ = Observable
.combineLatest(this.hasContentPartners$, this.hasContentServices$, this.hasContentTens$, this.hasContentWorklist$)
.map(oks => oks.some(ii => ii));
ngOnDestroy() {
[this.showPartners$, this.showServices$, this.showTens$, this.showWorklist$].forEach(ii => ii.complete());
}
gotoPartner = (id: string) => routing.gotoPartnerItem(this.router, id);
gotoProcess = (id: number) => routing.gotoProcess(this.router, id);
gotoTensItem = (id: DeliveryPointId) => routing.gotoTensItem(this.router, id);
gotoWorkitem = (id: number) => routing.gotoWorkitem(this.router, id);
}