From 0e9ffb76321da6dad9261c14c8393b5acd829405 Mon Sep 17 00:00:00 2001 From: Artur Androsovych Date: Mon, 30 Mar 2020 15:27:32 +0300 Subject: [PATCH] fix: create different subjects when `destroyMethodName` is provided (#70) --- integration/app/app.component.spec.ts | 126 ++++++++++++++---- .../issue-sixty-six.component.html | 6 +- .../issue-sixty-six.component.ts | 49 +++++-- src/lib/internals.ts | 36 +++-- src/lib/until-destroy.ts | 3 +- src/lib/until-destroyed.ts | 20 ++- 6 files changed, 185 insertions(+), 55 deletions(-) diff --git a/integration/app/app.component.spec.ts b/integration/app/app.component.spec.ts index 3323c26..7d251e8 100644 --- a/integration/app/app.component.spec.ts +++ b/integration/app/app.component.spec.ts @@ -221,6 +221,55 @@ describe('until-destroy runtime behavior', () => { expect(service.disposed).toBeTruthy(); }); + describe('https://github.com/ngneat/until-destroy/issues/61', () => { + it('should unsubscribe from streams if component inherits another directive or component', () => { + // Arrange + let baseDirectiveSubjectUnsubscribed = false, + mockComponentSubjectUnsubscribed = false; + + @Directive() + abstract class BaseDirective { + constructor() { + new Subject() + .pipe( + untilDestroyed(this), + finalize(() => (baseDirectiveSubjectUnsubscribed = true)) + ) + .subscribe(); + } + } + + @UntilDestroy() + @Component({ template: '' }) + class MockComponent extends BaseDirective { + constructor() { + super(); + + new Subject() + .pipe( + untilDestroyed(this), + finalize(() => (mockComponentSubjectUnsubscribed = true)) + ) + .subscribe(); + } + } + + // Act + TestBed.configureTestingModule({ + declarations: [MockComponent] + }); + + const fixture = TestBed.createComponent(MockComponent); + + // Assert + expect(baseDirectiveSubjectUnsubscribed).toBeFalsy(); + expect(mockComponentSubjectUnsubscribed).toBeFalsy(); + fixture.destroy(); + expect(baseDirectiveSubjectUnsubscribed).toBeTruthy(); + expect(mockComponentSubjectUnsubscribed).toBeTruthy(); + }); + }); + describe('https://github.com/ngneat/until-destroy/issues/66', () => { it('should be able to re-use methods of the singleton service multiple times', () => { // Arrange @@ -278,54 +327,79 @@ describe('until-destroy runtime behavior', () => { expect(startCalledTimes).toBe(2); expect(originalStopCalledTimes).toBe(2); }); - }); - describe('https://github.com/ngneat/until-destroy/issues/61', () => { - it('should unsubscribe from streams if component inherits another directive or component', () => { + it('should not use the single subject for different streams when `destroyMethodName` is provided', () => { // Arrange - let baseDirectiveSubjectUnsubscribed = false, - mockComponentSubjectUnsubscribed = false; + @Injectable() + class IssueSixtySixService { + firstSubjectUnsubscribed = false; + secondSubjectUnsubscribed = false; - @Directive() - abstract class BaseDirective { - constructor() { + startFirst(): void { new Subject() .pipe( - untilDestroyed(this), - finalize(() => (baseDirectiveSubjectUnsubscribed = true)) + untilDestroyed(this, 'stopFirst'), + finalize(() => (this.firstSubjectUnsubscribed = true)) ) .subscribe(); } - } - @UntilDestroy() - @Component({ template: '' }) - class MockComponent extends BaseDirective { - constructor() { - super(); + stopFirst(): void {} + startSecond(): void { new Subject() .pipe( - untilDestroyed(this), - finalize(() => (mockComponentSubjectUnsubscribed = true)) + untilDestroyed(this, 'stopSecond'), + finalize(() => (this.secondSubjectUnsubscribed = true)) ) .subscribe(); } + + stopSecond(): void {} + } + + @Component({ template: '' }) + class IssueSixtySixComponent { + constructor(private issueSixtySixService: IssueSixtySixService) {} + + startFirst(): void { + this.issueSixtySixService.startFirst(); + } + + stopFirst(): void { + this.issueSixtySixService.stopFirst(); + } + + startSecond(): void { + this.issueSixtySixService.startSecond(); + } + + stopSecond(): void { + this.issueSixtySixService.stopSecond(); + } } // Act TestBed.configureTestingModule({ - declarations: [MockComponent] + declarations: [IssueSixtySixComponent], + providers: [IssueSixtySixService] }); - const fixture = TestBed.createComponent(MockComponent); + const fixture = TestBed.createComponent(IssueSixtySixComponent); + const issueSixtySixService = TestBed.inject(IssueSixtySixService); - // Assert - expect(baseDirectiveSubjectUnsubscribed).toBeFalsy(); - expect(mockComponentSubjectUnsubscribed).toBeFalsy(); - fixture.destroy(); - expect(baseDirectiveSubjectUnsubscribed).toBeTruthy(); - expect(mockComponentSubjectUnsubscribed).toBeTruthy(); + fixture.componentInstance.startFirst(); + fixture.componentInstance.startSecond(); + fixture.componentInstance.stopFirst(); + + // Arrange + expect(issueSixtySixService.firstSubjectUnsubscribed).toBeTruthy(); + // Ensure that they listen to different subjects and all streams + // are not completed together. + expect(issueSixtySixService.secondSubjectUnsubscribed).toBeFalsy(); + + fixture.componentInstance.stopSecond(); + expect(issueSixtySixService.secondSubjectUnsubscribed).toBeTruthy(); }); }); }); diff --git a/integration/app/issue-sixty-six/issue-sixty-six.component.html b/integration/app/issue-sixty-six/issue-sixty-six.component.html index b562cee..f86cb8b 100644 --- a/integration/app/issue-sixty-six/issue-sixty-six.component.html +++ b/integration/app/issue-sixty-six/issue-sixty-six.component.html @@ -1,3 +1,5 @@ - - + + + +
diff --git a/integration/app/issue-sixty-six/issue-sixty-six.component.ts b/integration/app/issue-sixty-six/issue-sixty-six.component.ts index 38d27aa..3c41cda 100644 --- a/integration/app/issue-sixty-six/issue-sixty-six.component.ts +++ b/integration/app/issue-sixty-six/issue-sixty-six.component.ts @@ -5,16 +5,39 @@ import { finalize } from 'rxjs/operators'; @Injectable({ providedIn: 'root' }) export class IssueSixtySixService { - start(): void { + startFirst(): void { interval(1000) .pipe( - untilDestroyed(this, 'stop'), - finalize(() => console.log('IssueSixtySixService interval has been unsubscribed')) + untilDestroyed(this, 'stopFirst'), + finalize(() => + console.log( + 'IssueSixtySixService.startFirst() interval stream has been unsubscribed' + ) + ) ) - .subscribe(value => console.log(`IssueSixtySixService has emitted value ${value}`)); + .subscribe(value => + console.log(`IssueSixtySixService.startFirst() has emitted value ${value}`) + ); } - stop(): void {} + stopFirst(): void {} + + startSecond(): void { + interval(1000) + .pipe( + untilDestroyed(this, 'stopSecond'), + finalize(() => + console.log( + 'IssueSixtySixService.startSecond() interval stream has been unsubscribed' + ) + ) + ) + .subscribe(value => + console.log(`IssueSixtySixService.startSecond() has emitted value ${value}`) + ); + } + + stopSecond(): void {} } @Component({ @@ -24,11 +47,19 @@ export class IssueSixtySixService { export class IssueSixtySixComponent { constructor(private issueSixtySixService: IssueSixtySixService) {} - start(): void { - this.issueSixtySixService.start(); + startFirst(): void { + this.issueSixtySixService.startFirst(); + } + + stopFirst(): void { + this.issueSixtySixService.stopFirst(); + } + + startSecond(): void { + this.issueSixtySixService.startSecond(); } - stop(): void { - this.issueSixtySixService.stop(); + stopSecond(): void { + this.issueSixtySixService.stopSecond(); } } diff --git a/src/lib/internals.ts b/src/lib/internals.ts index fb0ff23..7114f78 100644 --- a/src/lib/internals.ts +++ b/src/lib/internals.ts @@ -12,15 +12,31 @@ export function isFunction(target: unknown) { } /** - * Applied to instances and stores `Subject` instance + * Applied to instances and stores `Subject` instance when + * no custom destroy method is provided. */ -export const DESTROY: unique symbol = Symbol('__destroy'); +const DESTROY: unique symbol = Symbol('__destroy'); /** * Applied to definitions and informs that class is decorated */ const DECORATOR_APPLIED: unique symbol = Symbol('__decoratorApplied'); +/** + * If we use the `untilDestroyed` operator multiple times inside the single + * instance providing different `destroyMethodName`, then all streams will + * subscribe to the single subject. If any method is invoked, the subject will + * emit and all streams will be unsubscribed. We wan't to prevent this behavior, + * thus we store subjects under different symbols. + */ +export function getSymbol(destroyMethodName?: keyof T): symbol { + if (typeof destroyMethodName === 'string') { + return Symbol(`__destroy__${destroyMethodName}`); + } else { + return DESTROY; + } +} + export function markAsDecorated( providerOrDef: InjectableType | DirectiveDef | ComponentDef ): void { @@ -46,19 +62,19 @@ export function ensureClassIsDecorated(instance: any): never | void { } } -export function createSubjectOnTheInstance(instance: any): void { - if (!instance[DESTROY]) { - instance[DESTROY] = new Subject(); +export function createSubjectOnTheInstance(instance: any, symbol: symbol): void { + if (!instance[symbol]) { + instance[symbol] = new Subject(); } } -export function completeSubjectOnTheInstance(instance: any): void { - if (instance[DESTROY]) { - instance[DESTROY].next(); - instance[DESTROY].complete(); +export function completeSubjectOnTheInstance(instance: any, symbol: symbol): void { + if (instance[symbol]) { + instance[symbol].next(); + instance[symbol].complete(); // We also have to re-assign this property thus in the future // we will be able to create new subject on the same instance. - instance[DESTROY] = null; + instance[symbol] = null; } } diff --git a/src/lib/until-destroy.ts b/src/lib/until-destroy.ts index cdb4756..37b6cdc 100644 --- a/src/lib/until-destroy.ts +++ b/src/lib/until-destroy.ts @@ -6,6 +6,7 @@ import { import { getDef, + getSymbol, isFunction, UntilDestroyOptions, completeSubjectOnTheInstance, @@ -31,7 +32,7 @@ function decorateNgOnDestroy( // It's important to use `this` instead of caching instance // that may lead to memory leaks - completeSubjectOnTheInstance(this); + completeSubjectOnTheInstance(this, getSymbol()); // Check if subscriptions are pushed to some array if (arrayName) { diff --git a/src/lib/until-destroyed.ts b/src/lib/until-destroyed.ts index fbebf6f..9121082 100644 --- a/src/lib/until-destroyed.ts +++ b/src/lib/until-destroyed.ts @@ -2,14 +2,18 @@ import { Observable } from 'rxjs'; import { takeUntil } from 'rxjs/operators'; import { - DESTROY, + getSymbol, isFunction, createSubjectOnTheInstance, completeSubjectOnTheInstance, ensureClassIsDecorated } from './internals'; -function overrideNonDirectiveInstanceMethod(instance: any, destroyMethodName: string): void { +function overrideNonDirectiveInstanceMethod( + instance: any, + destroyMethodName: string, + symbol: symbol +): void { const originalDestroy = instance[destroyMethodName]; if (isFunction(originalDestroy) === false) { @@ -18,11 +22,11 @@ function overrideNonDirectiveInstanceMethod(instance: any, destroyMethodName: st ); } - createSubjectOnTheInstance(instance); + createSubjectOnTheInstance(instance, symbol); instance[destroyMethodName] = function() { isFunction(originalDestroy) && originalDestroy.apply(this, arguments); - completeSubjectOnTheInstance(this); + completeSubjectOnTheInstance(this, symbol); // We have to re-assign this property back to the original value. // If the `untilDestroyed` operator is called for the same instance // multiple times, then we will be able to get the original @@ -33,15 +37,17 @@ function overrideNonDirectiveInstanceMethod(instance: any, destroyMethodName: st export function untilDestroyed(instance: T, destroyMethodName?: keyof T) { return (source: Observable) => { + const symbol = getSymbol(destroyMethodName); + // If `destroyMethodName` is passed then the developer applies // this operator to something non-related to Angular DI system if (typeof destroyMethodName === 'string') { - overrideNonDirectiveInstanceMethod(instance, destroyMethodName); + overrideNonDirectiveInstanceMethod(instance, destroyMethodName, symbol); } else { ensureClassIsDecorated(instance); - createSubjectOnTheInstance(instance); + createSubjectOnTheInstance(instance, symbol); } - return source.pipe(takeUntil((instance as any)[DESTROY])); + return source.pipe(takeUntil((instance as any)[symbol])); }; }