-2

I have a simple auth service and another component Foo which does the following:

  • fetch some data (two lists)
  • with these lists, subscribe to the auth login observable to fetch some more data
export class AuthService implements OnInit {
  login: BehaviorSubject<Login | undefined> = new BehaviorSubject<Login | undefined>(undefined);
  public readonly login$: Observable<Login | undefined> = this.login.asObservable();

    constructor(private httpService: HttpHandlerService) { }

    ngOnInit(): void {
        this.httpService.login().subscribe(
            (login: Login) => {
                this.login.next(login);
            });
    }
}

export class Foo implements OnInit {
  subscription?: Subscription;
  listA?: KeyValuePair[];
  listB?: KeyValuePair[];

  constructor(private httpHandlerCached: HttpHandlerCachedService, private auth: AuthService) {
  }

  ngOnInit(): void {
    this.initLists().subscribe(([listA, listB]) => {
      this.listA = listA;
      this.listB = listB;

      this.initModule();
    });
  }

  initLists(): Observable<[KeyValuePair[], KeyValuePair[]]> {
    return forkJoin([
      this.httpHandlerCached.getAllListA(),
      this.httpHandlerCached.getAllListB()
    ]);
  }

  initModule(): void {
    this.auth.login$.subscribe((login) => {
      if (login) {
        this.httpHandler.getSomeData(login.id)
          .subscribe(someData => doSomeStuff(someData, listA, listB));
      }
    })
  }
}

My attempt:

export class Foo implements OnInit, OnDestroy {
  listA?: KeyValuePair[];
  listB?: KeyValuePair[];
  subscription?: Subscription;

  constructor(private httpHandlerCached: HttpHandlerCachedService, private auth: AuthService) { }

  ngOnInit(): void {
    this.subscription = this.initLists().subscribe(([listA, listB]) => {
      this.listA = listA;
      this.listB = listB;

      this.initModule();
    });
  }

  initLists(): Observable<[KeyValuePair[], KeyValuePair[]]> {
    return forkJoin([
      this.httpHandlerCached.getAllListA(),
      this.httpHandlerCached.getAllListB()
    ]);
  }

  initModule(): void {
    this.subscription = this.auth.login$.subscribe((login) => {
      if (login) {
        this.httpHandler.getSomeData(login.id)
          .subscribe(someData => doSomeStuff(someData, listA, listB));
      }
    })
  }

  ngOnDestroy() {
    this.subscription?.unsubscribe();
  }
}

My questions:

  • is this the correct way to do it, to assign both subscriptions?
  • do I need to unsubscribe in both cases at all?

Thanks!

  • You are overwriting this.subscription in the second call so that’s not going to work anyway. However, the entire approach is not really correct. You should never nest subscriptions really. Read up on switchMap and other rxjs operators. A switchMap in this scenario can prevent the nested subscription all together. – MikeOne Sep 26 '21 at 10:54

1 Answers1

1

That's one of reasons why we avoid nested subscriptions - it makes cleanup messy. Instead use some operators inside the pipe, for example

this.auth.login$.pipe(
    filter(Boolean),
    switchMap(({ id }) => this.httpHandler.getSomeData(id)),
).subscribe(someData => doSomeStuff(someData, listA, listB));

Now there's only one subscription. You can unsubscribe on destroy, you can use the takeUntil pattern (Angular RxJS Observable: takeUntil vs. unsubscribe with a Subscription), maybe you can even avoid explicit subscription by using the async pipe in the template, dependent of what doSomeStuff actually does.

mbojko
  • 13,503
  • 1
  • 16
  • 26