Code review stories: subscription hell

“Volcano” by dmsumon is licensed under CC BY 2.0. To view a copy of this license, visit https://creativecommons.org/licenses/by/2.0/?ref=openverse.

While doing a code review I found this code which looks correct:

<div *ngIf="showWinningNotification$ | async">...</div>

It is the implementation of showWinningNotification$ that made me stop and think:

🤔

  public get showWinningNotification$(): Observable<boolean> {
    return this.featureToggleService.hasFeature$('winningNotificationOverlay');
  }

I’m sure I’ve seen this pattern multiple times in our code, and I have a feeling that it is not very efficient.

🤓

But, what’s the problem with it?

Well, in this case, showWinningNotification$ is a getter of a component with the default change detection strategy, which means that the code above is going to be executed on every change detection cycle.

If you use the default zone.js settings, this means it’s going to be executed a lot, all the time, on every mouse move, every time you breathe.

Let’s return to the template:

<div *ngIf="showWinningNotification$ | async">...</div>

The async operator is going to subscribe to the Observable returned by the getter, and then, in the next change detection cycle, it is going to unsubscribe from it so that it can subscribe again to the new Observable that was returned by the getter when it was called for the second time. And so on…

Let’s add some logging to verify our hypothesis:

  public get showWinningNotification$(): Observable<boolean> {
    return this.featureToggleService.hasFeature$('winningNotificationOverlay').pipe(
      switchMap(
        (value: boolean) =>
          new Observable<boolean>(subscriber => {
            console.log('debug: subscribing');
            subscriber.next(value);
            return () => console.log('debug: unsubscribing');
          })
      )
    );
  }

This was the result of my test in the dev console:

app-shell.component.ts:190 debug: unsubscribing
app-shell.component.ts:188 debug: subscribing
app-shell.component.ts:190 debug: unsubscribing
app-shell.component.ts:188 debug: subscribing
app-shell.component.ts:190 debug: unsubscribing
app-shell.component.ts:188 debug: subscribing
app-shell.component.ts:190 debug: unsubscribing
app-shell.component.ts:188 debug: subscribing
app-shell.component.ts:190 debug: unsubscribing
app-shell.component.ts:188 debug: subscribing
app-shell.component.ts:190 debug: unsubscribing
app-shell.component.ts:188 debug: subscribing
...
(and the list kept growing and growing)

Although, at first glance, it doesn’t seem that harmful, having a lot of these kinds of unwanted effects can damage the runtime performance of an Angular application. And they are difficult to spot.

🤓

So, what’s the alternative?

Use a normal class property instead:

  public readonly showWinningNotification$: Observable<boolean> = this.featureToggleService
    .hasFeature$('winningNotificationOverlay')
    .pipe(
      switchMap(
        (value: boolean) =>
          new Observable<boolean>(subscriber => {
            console.log('debug: subscribing');
            subscriber.next(value);
            return () => console.log('debug: unsubscribing');
          })
      )
    );

And it works like a charm.

app-shell.component.ts:188 debug: subscribing