feat(Subscription): add no longer returns unnecessary Subscription …

…reference (#5656)

- Updates a few locations where this feature was being used incorrectly.
- Eliminates confusion where people were trying to "chain" `add` calls.

BREAKING CHANGE: `add` no longer returns an unnecessary Subscription reference. This means that if you are calling `add` with a _function_ and not a `Subscription` (e.g. `subscription.add(() => { /* teardown */ })`), you will not be able to remove that teardown _function_ with `remove`. The fix for this is to wrap your function with a `Subscription` like so:  `const childSub = new Subscription(() => { /* teardown */ }); subscription.add(childSub);`. Then you will be able to remove it via `subscription.remove(childSub);`. Bear in mind that is it an edge case to need to manually remove a child subscription from a parent subscription. **All subscriptions that have been added to other subscriptions will remove themselves from the parent subscription(s) automatically when they are unsubscribed.**.

api_guard/dist/types/index.d.ts

@@ -548,7 +548,7 @@ export declare class Subscription implements SubscriptionLike {
     protected _parentOrParents: Subscription | Subscription[] | null;
     closed: boolean;
     constructor(unsubscribe?: () => void);
-    add(teardown: TeardownLogic): Subscription;
+    add(teardown: TeardownLogic): void;
     remove(subscription: Subscription): void;
     unsubscribe(): void;
     static EMPTY: Subscription;

spec/Subscription-spec.ts

@@ -4,85 +4,50 @@ import { Observable, UnsubscriptionError, Subscription, merge } from 'rxjs';
 /** @test {Subscription} */
 describe('Subscription', () => {
   describe('Subscription.add()', () => {
-    it('Should return self if the self is passed', () => {
-      const sub = new Subscription();
-      const ret = sub.add(sub);
-
-      expect(ret).to.equal(sub);
-    });
-
-    it('Should return Subscription.EMPTY if it is passed', () => {
-      const sub = new Subscription();
-      const ret = sub.add(Subscription.EMPTY);
-
-      expect(ret).to.equal(Subscription.EMPTY);
-    });
-
-    it('Should return Subscription.EMPTY if it is called with `void` value', () => {
-      const sub = new Subscription();
-      const ret = sub.add(undefined);
-      expect(ret).to.equal(Subscription.EMPTY);
-    });
-
-    it('Should return a new Subscription created with teardown function if it is passed a function', () => {
-      const sub = new Subscription();
-
+    it('should unsubscribe child subscriptions', () => {
+      const main = new Subscription();
+
       let isCalled = false;
-      const ret = sub.add(function() {
+      const child = new Subscription(() => {
         isCalled = true;
       });
-      ret.unsubscribe();
+      main.add(child);
+      main.unsubscribe();
 
       expect(isCalled).to.equal(true);
     });
 
-    it('Should wrap the AnonymousSubscription and return a subscription that unsubscribes and removes it when unsubbed', () => {
-      const sub: any = new Subscription();
-      let called = false;
-      const arg = {
-        unsubscribe: () => called = true,
-      };
-      const ret = sub.add(arg);
-
-      expect(called).to.equal(false);
-      expect(sub._subscriptions.length).to.equal(1);
-      ret.unsubscribe();
-      expect(called).to.equal(true);
-      expect(sub._subscriptions.length).to.equal(0);
-    });
-
-    it('Should return the passed one if passed a AnonymousSubscription having not function `unsubscribe` member', () => {
-      const sub = new Subscription();
-      const arg = {
-        isUnsubscribed: false,
-        unsubscribe: undefined as any,
-      };
-      const ret = sub.add(arg as any);
-
-      expect(ret).to.equal(arg);
-    });
-
-    it('Should return the passed one if the self has been unsubscribed', () => {
+    it('should unsubscribe child subscriptions if it has already been unsubscribed', () => {
       const main = new Subscription();
       main.unsubscribe();
 
-      const child = new Subscription();
-      const ret = main.add(child);
+      let isCalled = false;
+      const child = new Subscription(() => {
+        isCalled = true;
+      });
+      main.add(child);
 
-      expect(ret).to.equal(child);
+      expect(isCalled).to.equal(true);
     });
 
-    it('Should unsubscribe the passed one if the self has been unsubscribed', () => {
+    it('should unsubscribe a teardown function that was passed', () => {
+      let isCalled = false;
       const main = new Subscription();
+      main.add(() => {
+        isCalled = true;
+      });
       main.unsubscribe();
+      expect(isCalled).to.be.true;
+    });
 
+    it('should unsubscribe a teardown function that was passed immediately if it has been unsubscribed', () => {
       let isCalled = false;
-      const child = new Subscription(() => {
+      const main = new Subscription();
+      main.unsubscribe();
+      main.add(() => {
         isCalled = true;
       });
-      main.add(child);
-
-      expect(isCalled).to.equal(true);
+      expect(isCalled).to.be.true;
     });
   });
 

src/internal/Subscription.ts

@@ -140,11 +140,11 @@ export class Subscription implements SubscriptionLike {
    * `remove()` to remove the passed teardown logic from the inner subscriptions
    * list.
    */
-  add(teardown: TeardownLogic): Subscription {
+  add(teardown: TeardownLogic): void {
     let subscription = (<Subscription>teardown);
 
     if (!teardown) {
-      return Subscription.EMPTY;
+      return;
     }
 
     switch (typeof teardown) {
@@ -153,10 +153,9 @@ export class Subscription implements SubscriptionLike {
       case 'object':
         if (subscription === this || subscription.closed || typeof subscription.unsubscribe !== 'function') {
           // This also covers the case where `subscription` is `Subscription.EMPTY`, which is always in `closed` state.
-          return subscription;
+          return;
         } else if (this.closed) {
           subscription.unsubscribe();
-          return subscription;
         } else if (!(subscription instanceof Subscription)) {
           const tmp = subscription;
           subscription = new Subscription();
@@ -170,14 +169,14 @@ export class Subscription implements SubscriptionLike {
 
     // Add `this` as parent of `subscription` if that's not already the case.
     let { _parentOrParents } = subscription;
-    if (_parentOrParents === null) {
+    if (_parentOrParents == null) {
       // If we don't have a parent, then set `subscription._parents` to
       // the `this`, which is the common case that we optimize for.
       subscription._parentOrParents = this;
     } else if (_parentOrParents instanceof Subscription) {
       if (_parentOrParents === this) {
         // The `subscription` already has `this` as a parent.
-        return subscription;
+        return;
       }
       // If there's already one parent, but not multiple, allocate an
       // Array to store the rest of the parent Subscriptions.
@@ -187,7 +186,7 @@ export class Subscription implements SubscriptionLike {
       _parentOrParents.push(this);
     } else {
       // The `subscription` already has `this` as a parent.
-      return subscription;
+      return;
     }
 
     // Optimize for the common case when adding the first subscription.
@@ -198,7 +197,7 @@ export class Subscription implements SubscriptionLike {
       subscriptions.push(subscription);
     }
 
-    return subscription;
+    return;
   }
 
   /**

src/internal/operators/exhaust.ts

@@ -77,7 +77,7 @@ class SwitchFirstSubscriber<T> extends SimpleOuterSubscriber<T, T> {
 
   protected _next(value: T): void {
     if (!this.innerSubscription) {
-      this.innerSubscription = this.add(innerSubscribe(value, new SimpleInnerSubscriber(this)));
+      this.add(this.innerSubscription = innerSubscribe(value, new SimpleInnerSubscriber(this)));
     }
   }
 

src/internal/operators/subscribeOn.ts

@@ -14,9 +14,9 @@ export interface DispatchArg<T> {
 
 class SubscribeOnObservable<T> extends Observable<T> {
   /** @nocollapse */
-  static dispatch<T>(this: SchedulerAction<T>, arg: DispatchArg<T>): Subscription {
+  static dispatch<T>(this: SchedulerAction<T>, arg: DispatchArg<T>) {
     const { source, subscriber } = arg;
-    return this.add(source.subscribe(subscriber));
+    this.add(source.subscribe(subscriber));
   }
 
   constructor(