3

I have two components: NewItemComponent and ListComponent. When I create new item inside corresponding component I notify ListComponent so it can refresh it's data model:

export class NewItemComponent implements OnInit {

  constructor(private itemService: ItemService, private notificationService: NotificationService) {
  }

  ngOnInit() {
  }

  createNewItem(item: Item) {
    this.itemService.persist(item).subscribe((response: Item) => {
      console.log(response);
      this.notificationService.notifyNewItemHasBeenCreated(response);
    });
  }
}


export class ListComponent implements OnInit {

  items: Item[];

  constructor(private listService: ListService, private notificationService: NotificationService) {
  }

  ngOnInit() {
    this.loadItems();

    this.notificationService.item$.subscribe((item) => {
      if (item != null) {
        this.loadItems();
      }
    })
  }

  loadItems(){
    this.istService.getItems().subscribe((data: Item[]) => {
      this.items= data;
      console.log(this.items);
    });
  }
}


@Injectable({
  providedIn: 'root'
})
export class NotificationService {

  private _item: BehaviorSubject<Item> = new BehaviorSubject<Item>(null);
  public  item$ = this._item.asObservable();

  constructor() {
  }

  notifyNewItemHasBeenCreated(item: Item) {
    this._item.next(item);
  }
}

What makes me worried is that loadItems() calls subscribe() multiple times. Is this ok or there is better way to refetch items based on notification?

  loadItems(){
    this.listService.getItems().subscribe((data: Item[]) => {
      this.items= data;
      console.log(this.items);
    });
  }

ListService returns Observable:

export class ListService {

  basePath = 'my-api.com';
  apiPath = "item";

  constructor(private httpClient: HttpClient) {
  }

  getItems(): Observable<Item[]> {
    return this.httpClient.get<Item[]>(this.basePath + '/' + this.apiPath);
  }
}

Thanks in advance and any help will be really appreciated.

tillias
  • 1,035
  • 2
  • 12
  • 30

5 Answers5

9

As an experiment, if you do the following:

this.httpClient.get("<some url>")
  .subscribe({
    next: () => {
      console.log("received response")
    },
    error: err => {
      console.log("error occurred")
    },
    complete: () => {
      console.log("subscription completed")
    },
  })

You should see:

received response
subscription completed

This means that the observable for each web request is being completed after the request finishes, and therefore it is safe to not unsubscribe, as this is automatically done at the completion of an observable.

Michael
  • 2,189
  • 16
  • 23
  • But it seems that subscriptions are created multiple times (each time I call loadItems()). Not really sure if this is good for performance – tillias Sep 19 '18 at 17:39
  • 1
    Each time you call loadItems you are making a new web request via HttpClient, which is a new observable - one way or another you need to subscribe to the new observable for the new request. – Michael Sep 19 '18 at 18:50
1

Edit:

After looking around I found these posts:
Is it necessary to unsubscribe from observables created by Http methods?
Prevent memory leaks in Angular 2?

The comments and answers explain HttpClient.get() subscriptions clean up after themselves so you don't need to unsubscribe from them. That means calling .subscribe() on them multiple times is fine.

Joseph Webber
  • 2,010
  • 1
  • 19
  • 38
1

If you subscribe call multiple time, than unsubscribe it when you component destroy.

Do change in your component like this :

import { Component, OnInit, OnDestroy } from '@angular/core';
import { Subscription } from 'rxjs';

export class ListComponent implements OnInit, OnDestroy {

  items: Item[];

  constructor(private listService: ListService, private notificationService: NotificationService) {
  }

  subscriptions: Subscription[] = [];

  ngOnInit() {
     this.subscriptions.push(this.loadItems());

     this.subscriptions.push(this.notificationService.item$.subscribe((item) => {
       if (item) {
         this.loadItems();
       }
     }));
  }

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

  loadItems(){
    this.istService.getItems().subscribe((data: Item[]) => {
      this.items= data;
      console.log(this.items);
  });
 }
}
Shashikant Devani
  • 2,298
  • 1
  • 12
  • 25
1

loadItems() calls subscribe only once.
What actually happens is that you call loadItems() multiple times in notificationService.item$'s subscription.
If you need NotificationService only for this purpose I would suggest you little refactoring.

new-item.component.ts

export class NewItemComponent {

  constructor(private itemService: ItemService, private listService: ListService) {
  }

  createNewItem(item: Item) {
    this.itemService.persist(item).subscribe((response: Item) => {
      this.listService.addItem(item);
    });
  }
}

list.service.ts

export class ListService {

  basePath = 'my-api.com';
  apiPath = 'item';

  private itemsSubject: BehaviorSubject<Item[]> = new BehaviorSubject<Item[]>([]);
  private items$: Observable<Item[]>;

  constructor(private httpClient: HttpClient) {
  }

  getItems(): Observable<Item[]> {
    const http$ = this.httpClient.get<Item[]>(this.basePath + '/' + this.apiPath);
    return combineLatest(http$, this.items$).pipe(
      map(([httpResponse: Item[], localItems: Item[]]) => httpResponse.concat(localItems)),
    );
  }

  addItem(item: Item) {
    this.itemsSubject.next([...this.itemsSubject.value, item]);
  }

}
Eluzive
  • 99
  • 6
  • Hi Eluzive. Thanks for the answer. Can you please describe why subscribe() is called only once? Is it lifecycle of Observable? – tillias Sep 19 '18 at 17:19
  • I mean `subscribe` is called only once in `loadItems` but at the end it will be called multiple times because you call `loadItems` multiple times in `notificationsService.item$`'s subscription. In my solution you can get rid of `NotificationService` because you gonna do have always up to date items list. – Eluzive Sep 19 '18 at 17:38
0

What you're doing right now is:

  1. Make new item, send to server
  2. Server sends success response with item to item service. It notifies notification service. You don't actually do anything with this item but verify it.
  3. Your notification service gets item, notifies your list component.
  4. Your list component checks if the item is valid, then sends another request to the server for the updated list
  5. List component subscribes to the list service for the updated list. Updates when it comes.

So in a nutshell, for every new item you make you make 2 requests to the server and get 2 responses.

You can see that there's alot of redundancy here. Let's try to simplify things:

  1. Make new item, send to server
  2. Server sends success response with updated item list. It will only send this item list if the item received was valid, otherwise send back an error response.
  3. Your notification service is subscribed to this and gets item list that is already verified by the server, sends your list service this new item list.
  4. List component updates view with new data.

Now you only make one request each time instead of two. And you don't have to deal with so many Observables anymore.

Mark
  • 139
  • 7
  • Unfortunately there is some server-side logic and server-side caching when creating item as well as concurrency. This means I need to persist in a separate call and reload items after persisting. Anyways thanks! – tillias Sep 19 '18 at 17:08
  • Regardless of that, there is quite a bit of redundancy in your code. For one, the root of your problem here is loadItems both makes the http request and makes a new subscription everytime. Have your notification service make the request itself upon getting the item, and when you get the item pass it off to a service Subject to be redirected to one-time initialized subscriptions in the components that need it. – Mark Sep 19 '18 at 17:19
  • I don't like idea to mix data provisioning and notification in one service -> separation of concept. Two UI components should be in my case as much more isolated as possible as well. What I would like to fix in this scenario is to avoid making new subscriptions – tillias Sep 19 '18 at 17:28
  • Like I keep saying, the root of your problem is loadItems() - it makes both the request and the subscription! Separate that, reread my last comment on how to, and you won't have this problem of making new subscriptions. – Mark Sep 19 '18 at 17:34
  • Sorry, I don't understand -> that is why I asked it here. I'm comming from non-functional programming world and it is very difficult for me to understand without code-listings – tillias Sep 19 '18 at 17:37