diff --git a/apps/integration/src/app/inheritance/inheritance.component.html b/apps/integration/src/app/inheritance/inheritance.component.html index 16526c6..e374689 100644 --- a/apps/integration/src/app/inheritance/inheritance.component.html +++ b/apps/integration/src/app/inheritance/inheritance.component.html @@ -11,13 +11,20 @@ with `@UntilDestroy()` but directive is not
  • - `Issue97Component` extends the from `Issue97Directive`. Directive is decorated with + `Issue97Component` extends from `Issue97Directive`. Directive is decorated with `@UntilDestroy()` but component is not
    • +
  • + `Issue175Component` extends from the `Issue175Directive`. Directive is decorated with + `@UntilDestroy()` and the `Issue175Component` implements its own `ngOnDestroy()` + method. The `Issue175Directive.ngOnDestroy()` will not be called, because + `Issue175Component.ngOnDestroy()` does not call `super.ngOnDestroy()`. +
    • +
    Issue#61: {{ issue61StatusText$ | async }} @@ -26,6 +33,10 @@
    Issue#97: {{ issue97StatusText$ | async }}
    + +
    + Issue#175: {{ issue175StatusText$ | async }} +
    @@ -35,4 +46,11 @@ + diff --git a/apps/integration/src/app/inheritance/inheritance.component.ts b/apps/integration/src/app/inheritance/inheritance.component.ts index 3f37f5c..ee72359 100644 --- a/apps/integration/src/app/inheritance/inheritance.component.ts +++ b/apps/integration/src/app/inheritance/inheritance.component.ts @@ -7,15 +7,16 @@ import { NotificationClass, NotificationText } from '../enums/notification.enum' @Component({ selector: 'app-inheritance', templateUrl: './inheritance.component.html', - changeDetection: ChangeDetectionStrategy.OnPush + changeDetection: ChangeDetectionStrategy.OnPush, }) export class InheritanceComponent { issue61Shown = true; issue97Shown = true; + issue175Shown = true; issue61Status$ = new BehaviorSubject({ directiveUnsubscribed: false, - componentUnsubscribed: false + componentUnsubscribed: false, }); issue61StatusClass$ = this.issue61Status$.pipe( @@ -39,7 +40,7 @@ export class InheritanceComponent { ); issue97Status$ = new BehaviorSubject({ - componentUnsubscribed: false + componentUnsubscribed: false, }); issue97StatusClass$ = this.issue97Status$.pipe( @@ -54,6 +55,20 @@ export class InheritanceComponent { ) ); + issue175Status$ = new BehaviorSubject({ componentUnsubscribed: false }); + + issue175StatusClass$ = this.issue175Status$.pipe( + map(({ componentUnsubscribed }) => + componentUnsubscribed ? NotificationClass.Success : NotificationClass.Danger + ) + ); + + issue175StatusText$ = this.issue175Status$.pipe( + map(({ componentUnsubscribed }) => + componentUnsubscribed ? NotificationText.Unsubscribed : NotificationText.Subscribed + ) + ); + toggleIssue61(): void { this.issue61Shown = !this.issue61Shown; } @@ -61,4 +76,8 @@ export class InheritanceComponent { toggleIssue97(): void { this.issue97Shown = !this.issue97Shown; } + + toggleIssue175(): void { + this.issue175Shown = !this.issue175Shown; + } } diff --git a/apps/integration/src/app/inheritance/inheritance.module.ts b/apps/integration/src/app/inheritance/inheritance.module.ts index 23d0627..f2c0bc7 100644 --- a/apps/integration/src/app/inheritance/inheritance.module.ts +++ b/apps/integration/src/app/inheritance/inheritance.module.ts @@ -6,6 +6,7 @@ import { InheritanceComponent } from './inheritance.component'; import { Issue61Component } from './issue-61/issue-61.component'; import { Issue97Component } from './issue-97/issue-97.component'; +import { Issue175Component } from './issue-175/issue-175.component'; @NgModule({ imports: [ @@ -13,10 +14,10 @@ import { Issue97Component } from './issue-97/issue-97.component'; RouterModule.forChild([ { path: '', - component: InheritanceComponent - } - ]) + component: InheritanceComponent, + }, + ]), ], - declarations: [InheritanceComponent, Issue61Component, Issue97Component] + declarations: [InheritanceComponent, Issue61Component, Issue97Component, Issue175Component], }) export class InheritanceModule {} diff --git a/apps/integration/src/app/inheritance/issue-175/issue-175.component.ts b/apps/integration/src/app/inheritance/issue-175/issue-175.component.ts new file mode 100644 index 0000000..5b39883 --- /dev/null +++ b/apps/integration/src/app/inheritance/issue-175/issue-175.component.ts @@ -0,0 +1,21 @@ +import { Directive, Component, OnDestroy } from '@angular/core'; +import { Subject } from 'rxjs'; +import { untilDestroyed, UntilDestroy } from '@ngneat/until-destroy'; + +@UntilDestroy() +@Directive() +export abstract class Issue175Directive {} + +@Component({ + selector: 'app-issue-175', + template: '', +}) +export class Issue175Component extends Issue175Directive implements OnDestroy { + constructor() { + super(); + + new Subject().pipe(untilDestroyed(this)).subscribe(); + } + + ngOnDestroy(): void {} +} diff --git a/libs/until-destroy/src/lib/checker.ts b/libs/until-destroy/src/lib/checker.ts new file mode 100644 index 0000000..a7a231a --- /dev/null +++ b/libs/until-destroy/src/lib/checker.ts @@ -0,0 +1,174 @@ +import { + NgZone, + ElementRef, + ɵgetLContext, + ɵɵdirectiveInject, + PLATFORM_ID, +} from '@angular/core'; +import { isPlatformServer } from '@angular/common'; +import { + asapScheduler, + catchError, + EMPTY, + filter, + from, + mergeMap, + MonoTypeOperatorFunction, + Observable, + observeOn, + Subject, +} from 'rxjs'; + +import { getSymbol } from './internals'; + +const CLEANUP = 7; + +/** + * This function is used within the `UntilDestroy` decorator and returns a + * new class that setups a checker that the `destroy$` subject doesn't have + * observers (usually `takeUntil`) once the view is removed. + * Note: this code will not be shipped into production since it's guarded with `ngDevMode`, + * this means it'll exist only in development mode. + */ +export function createSubjectUnsubscribedChecker(type: any) { + if (isAngularInTestMode()) { + return type; + } + + return class SubjectUnsubscribedChecker extends type { + constructor(...args: any[]) { + super(...args); + + try { + const ngZone = ɵɵdirectiveInject(NgZone); + const platformId = ɵɵdirectiveInject(PLATFORM_ID); + const { nativeElement } = ɵɵdirectiveInject(ElementRef); + + // The checker should be executed only in the browser. + if (isPlatformServer(platformId)) { + return; + } + + ngZone.runOutsideAngular(() => + from(Promise.resolve()) + .pipe( + mergeMap(() => { + const lContext = ɵgetLContext(nativeElement); + if (lContext === null) { + return EMPTY; + } + + const lCleanup = lContext.lView[CLEANUP] || (lContext.lView[CLEANUP] = []); + const cleanupHasBeenExecuted$ = new Subject< + Subject | null | undefined + >(); + lCleanup.push(() => { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore Type 'symbol' cannot be used as an index type. + const destroy$: Subject | null | undefined = this[getSymbol()]; + cleanupHasBeenExecuted$.next(destroy$); + cleanupHasBeenExecuted$.complete(); + }); + return cleanupHasBeenExecuted$; + }), + filter((destroy$): destroy$ is Subject => destroy$ != null), + leaveNgZone(ngZone), + observeOn(asapScheduler), + catchError(() => EMPTY) + ) + .subscribe(destroy$ => { + // Note: The `observed` property is available only in RxJS@7.2.0, which will throw + // an error in lower versions; that's why it's wrapped into braces. The `observers` + // property is also being deprecated. + const observed = destroy$['observed'] ?? destroy$['observers'].length > 0; + if (observed) { + console.warn(createMessage(this)); + } + }) + ); + } catch { + // Leave the catch statement as empty since we don't have to execute any error handling logic here. + } + } + }; +} + +function createMessage(context: any): string { + return ` + The ${context.constructor.name} still has subscriptions that haven't been unsubscribed. + This may happen if the class extends another class decorated with @UntilDestroy(). + The child class implements its own ngOnDestroy() method but doesn't call super.ngOnDestroy(). + Let's look at the following example: + + @UntilDestroy() + @Directive() + export abstract class BaseDirective {} + + @Component({ template: '' }) + export class ConcreteComponent extends BaseDirective implements OnDestroy { + constructor() { + super(); + + someObservable$.pipe(untilDestroyed(this)).subscribe(); + } + + ngOnDestroy(): void { + // Some logic here... + } + } + + The BaseDirective.ngOnDestroy() will not be called since Angular will call ngOnDestroy() + on the ConcreteComponent, but not on the BaseDirective. + + One of the solutions is to declare an empty ngOnDestroy method on the BaseDirective: + + @UntilDestroy() + @Directive() + export abstract class BaseDirective { + ngOnDestroy(): void {} + } + + @Component({ template: '' }) + export class ConcreteComponent extends BaseDirective implements OnDestroy { + constructor() { + super(); + + someObservable$.pipe(untilDestroyed(this)).subscribe(); + } + + ngOnDestroy(): void { + // Some logic here... + super.ngOnDestroy(); + } + } + `; +} + +/** Gets whether the code is currently running in a test environment. */ +function isAngularInTestMode(): boolean { + // We can't use `declare const` because it causes conflicts inside Google with the real typings + // for these symbols and we can't read them off the global object, because they don't appear to + // be attached there for some runners like Jest. + return ( + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + (typeof __karma__ !== 'undefined' && !!__karma__) || + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + (typeof jasmine !== 'undefined' && !!jasmine) || + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + (typeof jest !== 'undefined' && !!jest) || + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + (typeof Mocha !== 'undefined' && !!Mocha) + ); +} + +function leaveNgZone(ngZone: NgZone): MonoTypeOperatorFunction { + return function leaveNgZoneOperator(source: Observable) { + return new Observable(observer => + source.subscribe(value => ngZone.runOutsideAngular(() => observer.next(value))) + ); + }; +} diff --git a/libs/until-destroy/src/lib/until-destroy.ts b/libs/until-destroy/src/lib/until-destroy.ts index df8e44e..fbe1f6e 100644 --- a/libs/until-destroy/src/lib/until-destroy.ts +++ b/libs/until-destroy/src/lib/until-destroy.ts @@ -6,6 +6,7 @@ import { import { Subscription } from 'rxjs'; import { PipeType, isPipe } from './ivy'; +import { createSubjectUnsubscribedChecker } from './checker'; import { getSymbol, UntilDestroyOptions, @@ -13,6 +14,10 @@ import { markAsDecorated, } from './internals'; +// This will be provided through Terser global definitions by Angular CLI. This will +// help to tree-shake away the code unneeded for production bundles. +declare const ngDevMode: boolean; + function unsubscribe(property: unknown): void { if (property instanceof Subscription) { property.unsubscribe(); @@ -66,6 +71,9 @@ function decoratePipe(type: PipeType, options: UntilDestroyOptions): void } export function UntilDestroy(options: UntilDestroyOptions = {}): ClassDecorator { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore The TS compiler throws 'typeof SubjectUnsubscribedChecker' is assignable to the constraint of type + // 'TFunction', but 'TFunction' could be instantiated with a different subtype of constraint 'Function'. return (type: any) => { if (isPipe(type)) { decoratePipe(type, options); @@ -74,5 +82,9 @@ export function UntilDestroy(options: UntilDestroyOptions = {}): ClassDecorator } markAsDecorated(type); + + if (ngDevMode) { + return createSubjectUnsubscribedChecker(type); + } }; }