2

So, I'm a little unclear on the unsubscribe() method.

It seems like the proper place to do it is in the ngOnDestroy() life cycle method? So I did that in the code below. BUT, the deleteFile() method won't execute now?

I read the below in another thread: (is this correct?)

"You shouldn't manually unsubscribe, 90% of the time. In this specific case, you're using a service to fetch data. If this service uses an http call, and thus the HttpClient, the observable is already closed once the call is finished and closed observables are already unsubscribed."

So do I need to unsubscribe here or not? Thanks

...
export class UploadComponent implements OnInit, OnDestroy {
 private subscriptions: Subscription;
...

ngOnDestroy() {
 this.subscriptions.unsubscribe();
}

deleteFile(id: any) {
 if (window.confirm("Are you sure you want to delete this file?")) {

  const sub = this.fileCabinetService.deleteFile(id)
    .subscribe(
      res => {

        ...
      },
      error => {

        ...
      }
    );

  this.subscriptions.add(sub);
 }
}
Mark
  • 2,543
  • 6
  • 33
  • 44

2 Answers2

3

You have misunderstood how Subscription and add work.

When you subscribe to something, you get a Subscription object, to which you can call unsubscribe(). This is fine. However, you haven't assigned the value correctly. You assigned the Subscription to const sub, which you then pass in as the completion block to add.

Consider Subscription.add like a finally block of a try/catch. No matter what the result of the Subscription, when it is complete, the block passed to add will execute. Use this for any cleanup tasks.

subscriptions: Subscriptions[] = [];

ngOnDestroy() {
   subscriptions.forEach((sub) => sub.unsubscribe());
}

deleteFile(id: any) {
   const sub = this.fileCabinetService.deleteFile(id).subscribe(...);
   this.subscriptions.push(sub);
   sub.add(() => this.subscriptions.remove(sub));
}

To answer your question about when to unsubscribe, it really depends on what deleteFile does. If the deleteFile method internally signals with a value (or a set of values) and then is completed, the subscription is terminated automatically. If it is not completed and left dangling, then your subscription will persist.

Consider the scenario of:

WallClock.subscribe((time) => console.log(time));

This Subscription will never be terminated because time (presumably) will continue indefinitely. Instead, you will want to manually control when this is terminated. You can do this in a few ways:

/* Get the current time, but don't bother listening to updates. */
WallClock.pipe(take(1)).subscribe((time) => console.log(time));

/* Continually update with the current time as long as the component is on screen
   — Requires setting `this.staySubscribed = false` when you're leaving */
WallClock.pipe(takeWhile(() => this.staySubscribed)).subscribe((time) => console.log(time));

/* Continually update until you remember to unsubscribe
   — Requires remembering to unsubscribe and can get verbose with multiple subscriptions
   - Call `this.subscription.unsubscribe()` later */
this.subscription = WallClock.subscribe((time) => console.log(time));

If your deleteFile operates like this and continually reports values without a definite completion condition, you should make sure to terminate the subscription in some way. It is likely the case (based on the name of the function) that this is a self-terminating Subscription that will not require anything on your part. If you want to be really safe, the pipe(take(1)) will ensure it for you.

Ian MacDonald
  • 13,472
  • 2
  • 30
  • 51
1

With regard to unsubscribe, you should take a look at the takeUntil pattern outlined in this answer. It is the preferred way to unsubscribe and avoid memory leaks. You could refactor your code to something like this:

...
export class UploadComponent implements OnInit, OnDestroy {
   private ngUnsubscribe = new Subject();
...

ngOnDestroy() {
   this.ngUnsubscribe.next();
   this.ngUnsubscribe.complete();
}

deleteFile(id: any) {
   if (window.confirm("Are you sure you want to delete this file?")) {

   this.fileCabinetService.deleteFile(id)
     .pipe(takeUntil(this.ngUnsubscribe))
     .subscribe(
       res => {
        ...
      },
      error => {
        ...
      }
    );
 }
}

That having been said, if you are subscribing to an @angular/common/http observable, you don't need to unsubscribe because the http service calls complete() when data is returned. Take a look at this implementation in the xhr back-end for details:

if (response.ok) {
  responseObserver.next(response);
  // TODO(gdi2290): defer complete if array buffer until done
  responseObserver.complete();
  return;
}

So, for @angular/common/http requests, don't worry about it. For most others, follow the takeUntil pattern.

Dean
  • 2,084
  • 17
  • 23
  • Assuming the `FileCabinetService` directly returns the `Observable` from an `HTTP` service in the method `deleteFile(id)` that `observable object` is completed and requires no `unsubscribe` call? – Itanex Feb 26 '20 at 19:51
  • @Itanex correct. The framework will `complete` your observable for you so you don't have to unsubscribe. That said, I usually want to cancel my HTTP call if a user navigates to a new page anyway. So, I usually use the `takeUntil` pattern for that reason but I it depends on the page/route being called. – Dean Feb 27 '20 at 01:40
  • My use of RXJS is not as avarice as most, I have rolled it into my development skills only recently. I have seen `takeUntil` and understand its function. Though up until recently, when I needed a subscription to unsubscribe I would usually do the following `const sub = myFunction().subscribe(() => sub.unsubscribe())`. This is obviously a highlight and not the entire code. I use this pattern when doing things like sending a delete/update that is out of band to the angular lifecycle hooks. or the ASYNC pipe. Which as it turns out, is very few and far between – Itanex Feb 27 '20 at 17:23