feat(Subscription): remove will now remove any teardown by reference (#…

…5659)

* feat(Subscription): remove will now remove any teardown by reference

Previously, only `Subscription` instances could be removed from a `Subscription`. Now any valid `TeardownLogic` that has been added to a `Subscription` may be removed by passing the same instance to `add`.

For example:

```ts
const teardown = () => console.log('called');

const subscription = new Subscription();

subscription.add(teardown);
subscription.remove(teardown);
subscription.unsubscribe();

// Will log nothing
```

Similarly with "unsubscribables":

```ts
const unsubscribable = {
    unsubscribe() {
        console.log('called');
    }
};

const subscription = new Subscription();

subscription.add(unsubscribable);
subscription.remove(unsubscribable);
subscription.unsubscribe();

// Will log nothing
```

- Cleans up and refactors Subscription substantially.
- Adds a few additional tests around Subscription and checking for teardown registries and unregistries

* refactor: Address comments

- Addresses comments
- Refines code a little further, moving some work closer to where it needs to be (teardowns array will no longer be allocated before we see if we even need to add anything, etc)
- Rewrites conditionals in _addParent method.
- Fixes test that didn't really test anything

* refactor: Remove unused declarations

* chore: address comments

api_guard/dist/types/index.d.ts

@@ -545,11 +545,10 @@ export declare class Subscriber<T> extends Subscription implements Observer<T> {
 }
 
 export declare class Subscription implements SubscriptionLike {
-    protected _parentOrParents: Subscription | Subscription[] | null;
     closed: boolean;
     constructor(unsubscribe?: () => void);
     add(teardown: TeardownLogic): void;
-    remove(subscription: Subscription): void;
+    remove(teardown: Exclude<TeardownLogic, void>): void;
     unsubscribe(): void;
     static EMPTY: Subscription;
 }
@@ -561,7 +560,7 @@ export interface SubscriptionLike extends Unsubscribable {
 
 export declare type Tail<X extends any[]> = ((...args: X) => any) extends ((arg: any, ...rest: infer U) => any) ? U : never;
 
-export declare type TeardownLogic = Unsubscribable | Function | void;
+export declare type TeardownLogic = Subscription | Unsubscribable | Function | void;
 
 export declare function throwError(errorFactory: () => any): Observable<never>;
 export declare function throwError(error: any): Observable<never>;

spec/Subscriber-spec.ts

@@ -2,6 +2,7 @@ import { expect } from 'chai';
 import { SafeSubscriber } from 'rxjs/internal/Subscriber';
 import { Subscriber, Observable } from 'rxjs';
 import { asInteropSubscriber } from './helpers/interop-helper';
+import { getRegisteredTeardowns } from './helpers/subscription';
 
 /** @test {Subscriber} */
 describe('Subscriber', () => {
@@ -129,4 +130,40 @@ describe('Subscriber', () => {
     subscriber.unsubscribe();
     expect(count).to.equal(1);
   });
+
+  it('should close, unsubscribe, and unregister all teardowns after complete', () => {
+    let isUnsubscribed = false;
+    const subscriber = new Subscriber();
+    subscriber.add(() => isUnsubscribed = true);
+    subscriber.complete();
+    expect(isUnsubscribed).to.be.true;
+    expect(subscriber.closed).to.be.true;
+    expect(getRegisteredTeardowns(subscriber).length).to.equal(0);
+  });
+
+  it('should close, unsubscribe, and unregister all teardowns after error', () => {
+    let isTornDown = false;
+    const subscriber = new Subscriber({
+      error: () => {
+        // Mischief managed!
+        // Adding this handler here to prevent the call to error from 
+        // throwing, since it will have an error handler now.
+      }
+    });
+    subscriber.add(() => isTornDown = true);
+    subscriber.error(new Error('test'));
+    expect(isTornDown).to.be.true;
+    expect(subscriber.closed).to.be.true;
+    expect(getRegisteredTeardowns(subscriber).length).to.equal(0);
+  });
+
+
+  it('should teardown and unregister all teardowns after complete', () => {
+    let isTornDown = false;
+    const subscriber = new Subscriber();
+    subscriber.add(() => { isTornDown = true });
+    subscriber.complete();
+    expect(isTornDown).to.be.true;
+    expect(getRegisteredTeardowns(subscriber).length).to.equal(0);
+  });
 });

spec/Subscription-spec.ts

@@ -3,7 +3,7 @@ import { Observable, UnsubscriptionError, Subscription, merge } from 'rxjs';
 
 /** @test {Subscription} */
 describe('Subscription', () => {
-  describe('Subscription.add()', () => {
+  describe('add()', () => {
     it('should unsubscribe child subscriptions', () => {
       const main = new Subscription();
 
@@ -49,9 +49,73 @@ describe('Subscription', () => {
       });
       expect(isCalled).to.be.true;
     });
+
+    it('should unsubscribe an Unsubscribable when unsubscribed', () => {
+      let isCalled = false;
+      const main = new Subscription();
+      main.add({
+        unsubscribe() {
+          isCalled = true;
+        }
+      });
+      main.unsubscribe();
+      expect(isCalled).to.be.true;
+    });
+
+    it('should unsubscribe an Unsubscribable if it is already unsubscribed', () => {
+      let isCalled = false;
+      const main = new Subscription();
+      main.unsubscribe();
+      main.add({
+        unsubscribe() {
+          isCalled = true;
+        }
+      });
+      expect(isCalled).to.be.true;
+    });
+  });
+
+  describe('remove()', () => {
+    it('should remove added Subscriptions', () => {
+      let isCalled = false;
+      const main = new Subscription();
+      const child = new Subscription(() => {
+        isCalled = true;
+      });
+      main.add(child);
+      main.remove(child);
+      main.unsubscribe();
+      expect(isCalled).to.be.false;
+    });
+
+    it('should remove added functions', () => {
+      let isCalled = false;
+      const main = new Subscription();
+      const teardown = () => {
+        isCalled = true;
+      };
+      main.add(teardown);
+      main.remove(teardown);
+      main.unsubscribe();
+      expect(isCalled).to.be.false;
+    });
+
+    it('should remove added unsubscribables', () => {
+      let isCalled = false;
+      const main = new Subscription();
+      const unsubscribable = {
+        unsubscribe() {
+          isCalled = true;
+        }
+      }
+      main.add(unsubscribable);
+      main.remove(unsubscribable);
+      main.unsubscribe();
+      expect(isCalled).to.be.false;
+    });
   });
 
-  describe('Subscription.unsubscribe()', () => {
+  describe('unsubscribe()', () => {
     it('Should unsubscribe from all subscriptions, when some of them throw', done => {
       const tearDowns: number[] = [];
 

spec/helpers/subscription.ts

@@ -0,0 +1,10 @@
+/** @prettier */
+import { TeardownLogic } from 'rxjs';
+
+export function getRegisteredTeardowns(subscription: any): Exclude<TeardownLogic, void>[] {
+  if ('_teardowns' in subscription) {
+    return subscription._teardowns ?? [];
+  } else {
+    throw new TypeError('Invalid Subscription');
+  }
+}

spec/operators/delay-spec.ts

@@ -203,7 +203,7 @@ describe('delay operator', () => {
         tap({
           next() {
             const [[subscriber]] = subscribeSpy.args;
-            counts.push(subscriber._subscriptions.length);
+            counts.push(subscriber._teardowns.length);
           },
           complete() {
             expect(counts).to.deep.equal([1, 1]);

spec/operators/observeOn-spec.ts

@@ -103,8 +103,8 @@ describe('observeOn operator', () => {
         x => {
           // see #4106 - inner subscriptions are now added to destinations
           // so the subscription will contain an ObserveOnSubscriber and a subscription for the scheduled action
-          expect(subscription._subscriptions.length).to.equal(2);
-          const actionSubscription = subscription._subscriptions[1];
+          expect(subscription._teardowns.length).to.equal(2);
+          const actionSubscription = subscription._teardowns[1];
           expect(actionSubscription.state.notification.kind).to.equal('N');
           expect(actionSubscription.state.notification.value).to.equal(x);
           results.push(x);
@@ -113,10 +113,10 @@ describe('observeOn operator', () => {
         () => {
           // now that the last nexted value is done, there should only be a complete notification scheduled
           // the consumer will have been unsubscribed via Subscriber#_parentSubscription
-          expect(subscription._subscriptions.length).to.equal(1);
-          const actionSubscription = subscription._subscriptions[0];
+          expect(subscription._teardowns.length).to.equal(1);
+          const actionSubscription = subscription._teardowns[0];
           expect(actionSubscription.state.notification.kind).to.equal('C');
-          // After completion, the entire _subscriptions list is nulled out anyhow, so we can't test much further than this.
+          // After completion, the entire _teardowns list is nulled out anyhow, so we can't test much further than this.
           expect(results).to.deep.equal([1, 2, 3]);
           done();
         }

spec/operators/switchAll-spec.ts

@@ -231,9 +231,10 @@ describe('switchAll', () => {
       oStreamControl.next(n); // creates inner
       iStream.complete();
     });
-    // Expect one child of switch(): The oStream
+
+    // Expect one child of switchAll(): The oStream
     expect(
-      (<any>sub)._subscriptions[0]._subscriptions.length
+      (sub as any)._teardowns?.[0]._teardowns?.length
     ).to.equal(1);
     sub.unsubscribe();
   });
@@ -250,14 +251,14 @@ describe('switchAll', () => {
     [0, 1, 2, 3, 4].forEach((n) => {
       oStreamControl.next(n); // creates inner
     });
-    // Expect one child of switch(): The oStream
+    // Expect one child of switchAll(): The oStream
     expect(
-      (sub as any)._subscriptions[0]._subscriptions.length
+      (sub as any)._teardowns?.[0]._teardowns?.length
     ).to.equal(1);
     // Expect two children of subscribe(): The destination and the first inner
     // See #4106 - inner subscriptions are now added to destinations
     expect(
-      (sub as any)._subscriptions.length
+      (sub as any)._teardowns?.length
     ).to.equal(2);
     sub.unsubscribe();
   });