1

I am writing some code, in Typescript for Angular 11, where I need to get data from one observable and then looping through that have another call to get the name. Once I get the name I want to append it to the original object.

For example I have two observables and getSelfPurchases() returns:

{
  id: 32
  user_id: 5
  script_id: 78
  pp_id: "76SDI89768UIHUIH8976"
},
{
  id: 33
  user_id: 4
  script_id: 79
  pp_id: "78FGGF78SDF78FSD"
}

and the second one, getScriptDetails(32), returns:

{
  sname: "Spell Checker"
  author: 43
  price: 20.99
}

I have successfully achieved what I wanted to do but I feel like it is sloppy and inefficient. I've been reading more into RXJS stuff like switch map, but I am unsure if something like that can be done. Or maybe the method I chose is the best one already. Input?

this.userService.getSelfPurchases().subscribe(response => { // first observable
  this.purchases = response;

  this.purchases.forEach((purchase, index) => { // loop through our results
    this.scriptService.getScriptDetails(purchase.script_id).subscribe(responseTwo => { // second observable
      if (responseTwo[0] && responseTwo[0].sname !== undefined) {
        this.purchases[index].sname = responseTwo[0].sname; // append to our original purchases object
      }
    });
  });
});
awild
  • 49
  • 2
  • 9

3 Answers3

1

You basically never want to nest subscriptions. It's not a matter of efficiency so much as it's a matter so much as maintainability, extendability, and (most importantly) readability.

Nested subscriptions quickly lead to call-back hell. It's surprisingly simple to get hopelessly lost that way. Though there's a time and place for everything I suppose.

This is your code re-written 1-1 as you had it before, without nesting subscriptions. I map your array of purchases into an array of getScriptDetails calls and then subscribe to that array via merge.

this.userService.getSelfPurchases().pipe(
  tap(response => this.purchases = response),
  map(purchases => purchases.map((purchase, index) => 
    this.scriptService.getScriptDetails(purchase.script_id).pipe(
      map(responseTwo => ({index, responseTwo}))
    )
  )),
  mergeMap(scriptDetailsCalls => merge(...scriptDetailsCalls)),
).subscribe(({index, responseTwo}) => {
  if (responseTwo[0] && responseTwo[0].sname !== undefined) {
    // append to our original purchases object
    this.purchases[index].sname = responseTwo[0].sname; 
  }
});

You can combine the map and mergeMap above into a single mergeMap as follows:

this.userService.getSelfPurchases().pipe(
  tap(response => this.purchases = response),
  mergeMap(purchases => merge(...
    purchases.map((purchase, index) => 
      this.scriptService.getScriptDetails(purchase.script_id).pipe(
        map(responseTwo => ({index, responseTwo}))
      )
    ))
  )
).subscribe(({index, responseTwo}) => {
  if (responseTwo[0] && responseTwo[0].sname !== undefined) {
    // append to our original purchases object
    this.purchases[index].sname = responseTwo[0].sname; 
  }
});

Aside: Avoid global variables

It's a personal taste for functional "purity," but it's generally cleaner to avoid the pattern where you set a global variable and then modify it later. Makes testing it harder as it leaves you with fewer guarantees about the state of that global variable.

this.userService.getSelfPurchases().pipe(
  mergeMap(purchases => forkJoin(
    purchases.map(purchase => 
      this.scriptService.getScriptDetails(purchase.script_id).pipe(
        map(responseTwo => ({...purchase, sname: responseTwo[0].sname}))
      )
    )
  ))
).subscribe(purchasesWName =>
  this.purchases = purchasesWName
);
Mrk Sef
  • 7,557
  • 1
  • 9
  • 21
  • Updated for what I think is a cleaner version – Mrk Sef Dec 17 '20 at 17:49
  • Your answer was very insightful especially with your addition. I appreciate you taking your time to answer. – awild Dec 17 '20 at 17:51
  • Question though, I get a depreciation notice with fork join on your aside code. I guess forkJoin now takes an array of observables. Putting the code into brackets causes it not to complete. Any suggestion on that? – awild Dec 17 '20 at 18:30
  • `purchses.map(...)` returns an array of observables. Not sure why you'd get a deprecation notice for that. – Mrk Sef Dec 17 '20 at 18:34
1

it's a typical case swichMap, forkJoin, map

  1. First get the list
  2. Create an array of observables
  3. make the forkJoin
  4. map the initial list adding the values received

In code

this.userService.getSelfPurchases().pipe(
   switchMap(purchases=>{
         //here you has the purchases, e.g. [{script_id:2,script_id:4}]
         //so create an array with the observables
         const obs=purchases.map(purchase=>this.scriptService.getScriptDetails(purchase.script_id))
        //using switchmap we should return an observable
        return forkJoin(obs).pipe(
            //data is an array with the response of the call for script_id:2 and script_id4
            //but we don't want return only an array with the data
            //so we use map to transform the data
            map((data:any[])=>{
               //we loop over purchases to add the properties
               //we get in data
               purchases.forEach((purchase,index)=>{
                  purchase.sname=data[index].sname
                  purchase.author=data[index].author
                  purchase.price=data[index].price
                  purchase.author=data[index].author
               }
               //finally return purchases
               return purchases
            })
        )
   })
)
Eliseo
  • 50,109
  • 4
  • 29
  • 67
0

Subscriptions should not be nested.

There are possibilities to e.g. concat, forkJoin, switchMap or merge pipes for combining subscriptions.

Nesting subscriptions is an anti pattern: https://www.thinktecture.com/de/angular/rxjs-antipattern-1-nested-subs/

RxJs Observables nested subscriptions?

Guntram
  • 961
  • 14
  • 19
  • (I have to admit that I am maintaining some nested subscriptions, I managed to pipe some together, but it is more difficult to refactor nested subscriptions than piping them right from the beginning) – Guntram Dec 17 '20 at 16:05