CODEWITHSUNDEEP
rxjs
dwjohnston
dwjohnston

Reputation: 11890

observable.subscribe(..).unsubscribe(); anti-pattern.

I've been finding myself writing code that looks like this: 

class WidgetService {

   getWidgets() {
       return this.authService.user.pipe(  //authService.user is an Observable, that emits the currently authenticated users. 
         first(user => user!=null),  //Make sure a null object isn't coming through
         switchMap(user => {
            return this.collection.getWhere("widget.ownerId", "==", user.id); //Get all the widgets for that user
         })
       ); 
   }

}

class WidgetDisplayComponent {

    ngOnInit() {
       this.widgetService.getWidget().subscribe(widget => this.widget = widget).unsubscribe(); //Subscribe to cause the Observable to pipe, get the widget, then unsubscribe. 
    }
}

Is this an anti pattern? What should I be doing instead, where the requirements are:

Upvotes: 2

Views: 514

Answers (2)

RagnarLothbrock
RagnarLothbrock

Reputation: 129

A bit late, but given Angular evolution this question is still interesting these days.

First let me introduce my solution

class WidgetDisplayComponent {

    private widgetService = inject(WidgetService);

    widgets: T = 'default value, always initialize if possible';

    getWidgets$ = this.widgetService
        .getWidget()
        // import { takeUntilDestroyed } from '@angular/core/rxjs-interop';
        .pipe(takeUntilDestroyed())
        .subscribe((widgets) => (this.widgets = widgets));
}

this solution is assuming that the observable will eventually emit one set of widgets. but if that is not the case, it should be responsibility of the service, so I'm not taking care to specify logic for that. For what concerns the view, getWidget is the method to retrieve the data that will be presented. Now let's focus on the code.

you'll notice a couple changes:

  1. I'm not using ngOnInit and I'm declaring the subscription in the injection context of the component. This has some benefits such as:

    a. I can use takeUntilDestroyed to cleanup my duscription

    b. I don't need to implement onInit and onDestroy hooks, which makes my code more readable

    c. I can declare and initialize the subscription in the same line.

One could think of ways to improve this, such as handling errors or empty collections, but the core logic is that. Just declare the subscription in the injection context and take advantage of the new features of angular, while reducing your code.

Now let me introduce an even more interesting case, What if you have a refresh widgets button, meaning that every time you click, you get a "refreshed" collection. In that case, we are forced to declare one method to call that logic, and my approach this time would be as follows:

async refreshWidget(){
        try{
            const freshWidgets = await firstValueFrom(this.widgetService.getWidget());
      this.widgets = freshWidgets;
        }catch(e){
            // do something with the error
        }
    };

you can add more logic, to prevent double execution in case of multiple clicks in a given time period, but let's keep it simple for now.

The reason I'm using this approach is to avoid having to create a subscription holder and add every new instance to it. Also everyone is familiar with async await logic and the code looks readable and easy. This use case covers the use case of retrieving one value from an observable inside a method (matching the use case you had in your original code).

Both examples are focused in particular use cases:

  • single async execution at the beginning
  • multiple async executions programatically triggered.

Other use cases might require other solutions.

Hope it helps. Best regards.

Upvotes: 0

cartant
cartant

Reputation: 58430

I would say that it is absolutely an anti-pattern, for these reasons:

  • With synchronous sources, there is no guarantee that you will receive only one next notification.
  • With asynchronous sources, you won't receive any next notifications - as the subscriber will be synchronously unsubscribed.

The pattern would only work for synchronous sources that emit only a single next notification. In which case, the unsubscribe is redundant, as the subscriber will be automatically unsubscribed when the source completes.

IMO, if you know that the source emits only one next notification, you should just omit the unsubscribe. If you are unsure, you should use either first or take(1) at the subscription point.

There is another mechanism that can be used to unsubscribe upon receipt of the first next notification, but it's not one that I would encourage, as it requires a non-arrow function to be used.

The subscriber is used as the context when the next handler is invoked, so it's possible to call unsubscribe on it, like this:

source.subscribe(function (value) {
  /* do something with value */
  this.unsubscribe();
});

Upvotes: 3

Related Questions