0
\$\begingroup\$

Context

In an Angular application, I have three <select> boxes that are fed from the database with a relatively small data set. These boxes are part of a filter Component that is displayed next to a server-side data table. The component outputs the filter asynchronously to the parent component, which then pipes into the table, invokes server, filters out SQL etc.

Focusing on the scope, when the control is loaded, I want to invoke the server with three GET invocations. Until they complete, I want to display a spinner.

Since I am using a library of controls, part of the code is already done. I want to ask here if I am handling the Observables correctly from the Angular point of view

Code I wrote so far

The code works

export class MyComponent implements OnInit, OnDestroy {

  public filterLoading$: BehaviorSubject<boolean>;
  public select1: Observable<string[]>;
  public select2: Observable<string[]>;
  public select3: Observable<string[]>;
  private filterLoadingSub$?: Subscription;

  constructor(private remoteService: RemoteService) {
    this.select1 = remoteService.getFirst();
    this.select2 = remoteService.getSecond();
    this.select3 = remoteService.getThird();
    this.filterLoading$ = new BehaviorSubject<boolean>(true);
  }


  ngOnInit(): void {
    this.filterLoadingSub$ = forkJoin([this.select1, this.select2, this.select3])
      .subscribe(() => this.filterLoading$.next(false))
    ;
  }


  ngOnDestroy(): void {
    if (!!this.filterLoadingSub$) {
      this.filterLoadingSub$.unsubscribe();
    }
  }

}

Explanation:

  • selectX are bound to the front end with <option *ngFor="let x of (selectX | async)" ...>
  • The subject filterLoading$ is used to display the spinner. Actually my custom accordion handles it for me <acme-filter-panel isLoaded="!(filterLoading$ | async)>
  • Then I used forkJoin operator to wait for the completion of all three calls. I don't care about the last returned valued, only that the three have completed. But this leads to a Subscription
  • Under the philosophy of dispose your own garbage, every time I hit a Subscription I always write code to unsubscribe. However, these are just HTTP calls that return only once when invoked.

I truly understand that I am not handling error logic, but really that goes beyond the scope of the question

Questions

My question is about two aspects.

Apart from moving code from constructor to ngOnInit, on reasons of symmetry, am I writing too much code or is there some simplification I can use to say Angular to unlock the spinner when all of the three invocations occurred?

Is it always necessary to always unsubscribe from subscriptions? Or are there cases in which a dangling subscription harms none? I learned that subscriptions use memory, so I deem necessary to clean up resources whenever possible, even if modern machines have gigabytes of RAM.

More in general, is it possible to write this piece in a more concise way? For three data sets I had to instantiate a fourth member, which is actually needed to monitor the spinner, and a fifth member used only on destruction to clean up. I often work with Observables & co.

\$\endgroup\$
1
\$\begingroup\$

It is in theory not necessary to unsubscribe all subscriptions. For example if you call the backend with the Angular HttpClient then you will get an observable, that will automaticly complete after it emits the first result. As soon as it is completed, the observable is gone and does not strain your ressources anymore.
That means, if the Observable will complete itself in a timely manner, then you do not have to unsubscribe.

BUT: In most cases you have to dig a bit deeper to see that an observable will complete itself. That means the reader of your code will not easily know if you forgot the unsubscribe or if it could be skipped. Therefore i personaly decided for safety (but more code) and unsubscribe always.

Not unsubscribing will result in unnecessary memory consumption and/or can create unexpected sideeffects. The function that should be executed at emits, is stored at the Subject. That means even when your component is long ago destroyed, the function will still "live" at the Subject. And will be executed at each emit...

But you could make unsubscribing a bit nicer with a Subscription Object

private subscriptions: Subscription = new Subscription();
...
this.subscriptions.add( myOberservable.subscribe(value => doMagic(value)) );
this.subscriptions.add( myOtherOberservable.subscribe(value => doMoreMagic(value)) );
...
this.subscriptions.unsubribe() // will unsubscribe all added subscriptions at once

About the general question of implementing the spinner. If you always want a spinner, you could attach it in an interceptor, activating the spinner before the call is fired and deactivated as soon as you get a respond.
Or using a central method for backend calls and doing the the toggle there.
In both cases you would need a spinner service that needs to remember how often it was "activated", so that if 3 calls are triggered at once, the first response does not deactivate the trigger.

May ways are possible, it depends a bit on your use case and your general architecture.

But your specific example i would write in the following way:

export class MyComponent implements OnInit, OnDestroy {

  private subscriptions: Subscription = new Subscription();
  private filterLoading$:BehaviorSubject<boolean> = new BehaviorSubject<boolean>(true);

  ngOnInit(){
      this.subscriptions.add(this.callAll().subscribe(() => this.toggleSpinner()));
  }

  ngOnDestroy(){
      this.subscriptions.unsubscribe();
  }
  
  private callAll():Observable[]{
    const listOfCalls: Observable[] = [
          remoteService.getFirst(),
          remoteService.getSecond(),
          remoteService.getThird(),
    ];
    return forkJoin(listOfCalls);
  }

  private toggleSpinner():void{
      this.filterLoading.next(false)
  }
}

Its nearly the same way you used. Only a few changes.

  1. If there is no need to declare something public, then its private.
  2. I make use of the "Subscription" Object, even if in this example there is just one subscription to be stored. I try to be consisten with solutions. Also it makes it easier to extend the component when a second subscription is needed.
  3. I like to hide code behind methods. That way someone reads the methodname and then can decide if he/she is interested in the details or not.

All in all its more code. But i think its easier to understand. And in my experience, code is read quite more often then writen. And "read" really means "reading and understanding". And thats an very expensive task. :-)

As always, the solution totaly depends on the context. So, choose the things that suit you well and skip the rest. :-)

\$\endgroup\$
3
  • \$\begingroup\$ You just triple-confirmed about how good of an idea is to write good, clean, easy to understand and maintainable code versus cheap code that just works \$\endgroup\$ – usr-local-ΕΨΗΕΛΩΝ Mar 18 at 10:28
  • \$\begingroup\$ It is also interesting to see that both a single Observable and an array of Observables (Observable[]) offer a subscribe method, which is probably what I was looking for in my very beginning. About publicity of some members, they have to be bound on the template, and if they are not public they are not accepted by the transpiler. I am working on Angular since just 2 months. \$\endgroup\$ – usr-local-ΕΨΗΕΛΩΝ Mar 18 at 10:30
  • \$\begingroup\$ (no... I suggested an edit because you used forkJoin correctly in callAll, which returns a single observable from an array of those) \$\endgroup\$ – usr-local-ΕΨΗΕΛΩΝ Mar 18 at 13:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.