fix: unsubscribe only from actual subscriptions (#165)

ngneat · Aug 25, 2021 · 56914f2 · 56914f2
1 parent 284ed60
commit 56914f2
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 19 deletions.
diff --git a/libs/until-destroy/src/lib/internals.ts b/libs/until-destroy/src/lib/internals.ts
@@ -3,10 +3,6 @@ import { Subject } from 'rxjs';
 
 import { PipeType } from './ivy';
 
-export function isFunction(target: unknown) {
-  return typeof target === 'function';
-}
-
 /**
  * Applied to instances and stores `Subject` instance when
  * no custom destroy method is provided.

diff --git a/libs/until-destroy/src/lib/until-destroy.ts b/libs/until-destroy/src/lib/until-destroy.ts
@@ -1,21 +1,22 @@
 import {
   InjectableType,
   ɵComponentType as ComponentType,
-  ɵDirectiveType as DirectiveType
+  ɵDirectiveType as DirectiveType,
 } from '@angular/core';
-import { SubscriptionLike } from 'rxjs';
+import { Subscription } from 'rxjs';
 
 import { PipeType, isPipe } from './ivy';
 import {
   getSymbol,
-  isFunction,
   UntilDestroyOptions,
   completeSubjectOnTheInstance,
-  markAsDecorated
+  markAsDecorated,
 } from './internals';
 
-function unsubscribe(property: SubscriptionLike | undefined): void {
-  property && isFunction(property.unsubscribe) && property.unsubscribe();
+function unsubscribe(property: unknown): void {
+  if (property instanceof Subscription) {
+    property.unsubscribe();
+  }
 }
 
 function unsubscribeIfPropertyIsArrayLike(property: any[]): void {
@@ -26,7 +27,7 @@ function decorateNgOnDestroy(
   ngOnDestroy: (() => void) | null | undefined,
   options: UntilDestroyOptions
 ) {
-  return function(this: any) {
+  return function (this: any) {
     // Invoke the original `ngOnDestroy` if it exists
     ngOnDestroy && ngOnDestroy.call(this);
 

diff --git a/libs/until-destroy/src/lib/until-destroyed.ts b/libs/until-destroy/src/lib/until-destroyed.ts
@@ -4,9 +4,8 @@ import { takeUntil } from 'rxjs/operators';
 import {
   DECORATOR_APPLIED,
   getSymbol,
-  isFunction,
   createSubjectOnTheInstance,
-  completeSubjectOnTheInstance
+  completeSubjectOnTheInstance,
 } from './internals';
 
 // This will be provided through Terser global definitions by Angular CLI. This will
@@ -20,15 +19,15 @@ function overrideNonDirectiveInstanceMethod(
 ): void {
   const originalDestroy = instance[destroyMethodName];
 
-  if (ngDevMode && isFunction(originalDestroy) === false) {
+  if (ngDevMode && typeof originalDestroy !== 'function') {
     throw new Error(
       `${instance.constructor.name} is using untilDestroyed but doesn't implement ${destroyMethodName}`
     );
   }
 
   createSubjectOnTheInstance(instance, symbol);
 
-  instance[destroyMethodName] = function() {
+  instance[destroyMethodName] = function () {
     // eslint-disable-next-line prefer-rest-params
     originalDestroy.apply(this, arguments);
     completeSubjectOnTheInstance(this, symbol);

diff --git a/libs/until-destroy/tests/until-destroy.spec.ts b/libs/until-destroy/tests/until-destroy.spec.ts
@@ -1,6 +1,6 @@
 import {
   ɵComponentDef as ComponentDef,
-  ɵɵdefineComponent as defineComponent
+  ɵɵdefineComponent as defineComponent,
 } from '@angular/core';
 import { interval, Subject } from 'rxjs';
 
@@ -17,7 +17,7 @@ describe('UntilDestroy decorator alone', () => {
         decls: 0,
         type: TestComponent,
         selectors: [[]],
-        template: () => {}
+        template: () => {},
       }) as ComponentDef<TestComponent>;
 
       subscription = interval(1000).subscribe();
@@ -42,7 +42,7 @@ describe('UntilDestroy decorator alone', () => {
         decls: 0,
         type: TestComponent,
         selectors: [[]],
-        template: () => {}
+        template: () => {},
       }) as ComponentDef<TestComponent>;
 
       intervalSubscription = interval(1000).subscribe();
@@ -74,7 +74,7 @@ describe('UntilDestroy decorator alone', () => {
         decls: 0,
         type: TestComponent,
         selectors: [[]],
-        template: () => {}
+        template: () => {},
       }) as ComponentDef<TestComponent>;
 
       subscriptions = [interval(1000).subscribe(), new Subject().subscribe()];
@@ -95,4 +95,30 @@ describe('UntilDestroy decorator alone', () => {
       expect(subscription.closed).toBeTruthy();
     });
   });
+
+  it('should unsubscribe only from actual subscriptions', () => {
+    // Arrange
+    @UntilDestroy({ checkProperties: true })
+    class TestComponent {
+      static ɵcmp = defineComponent({
+        vars: 0,
+        decls: 0,
+        type: TestComponent,
+        selectors: [[]],
+        template: () => {},
+      }) as ComponentDef<TestComponent>;
+
+      subject$ = new Subject<never>();
+
+      static ɵfac = () => new TestComponent();
+    }
+
+    // Act & assert
+    const component = TestComponent.ɵfac();
+
+    expect(component.subject$.closed).toBe(false);
+    callNgOnDestroy(component);
+    // The `Subject` also has the `unsubscribe()` method, but it's not a subscription.
+    expect(component.subject$.closed).toBe(false);
+  });
 });