0

What is the correct way to write this without using a nested subscription?

The first observable will return a value that is passed to the second observable, then I need to use the returned values from both observables in the final subscription.

Also, the "doStuff" method is called from a button click, do I need to also unsubscribe from these observables to prevent memory leaks?

  doStuff() {

    // Get app user
    this.appUserService.getAppUser(true).subscribe((appUser) => {
      
      // Get login key
      this.accountApi.getLoginKey(appUser).subscribe(loginKey => {

        // Open in app browser
        this.browser = this.inAppBrowser.create(appUser.Url + loginKey, '_system');
      });
    });
  }

Edit 1

Tried the following as suggested but this does not compile, on the second pipe there is the following error:

Property 'pipe' does not exist on type 'OperatorFunction<AppUser, any>'

Then within the "map" function there is the following error:

Cannot find name 'appUser'.

   return this.appUserService.getAppUser(true).pipe(
      switchMap((appUser: AppUser) => this.accountApi.getUserLoginKey(appUser)).pipe(
        map(loginKey => { appUser, loginKey; }),
      ),
      tap(({ appUser, loginKey }) => {
        this.browser = this.inAppBrowser.create(appUser.Url + loginKey, '_system');
      }),
      take(1)
    );

Edit 2

I have now updated my solution as follows which works (but I could argue my original solution did work), so I have removed the nested subscriptions but they have been replaced with nested pipes instead. I believe this is a more acceptable solution (i.e. using nested "pipes" rather than nested "subscriptions") but not sure if it can be improved any further?

doStuff() {
       // Get app user
        this.appUserService.getAppUser(true).pipe(
          switchMap(appUser => this.accountApi.getLoginKey(appUser).pipe(
            tap(loginKey => {
              // Open in app browser as return URL
              this.browser = this.inAppBrowser.create(appUser.Url + loginKey, '_system');
            })
          )),
          take(1)
        ).subscribe();
}
WebFletch
  • 172
  • 1
  • 16

3 Answers3

2

The "correct" way all depends on the specific behaviour you're looking for. If you only want one request to be alive at a time, use switchMap() or exhaustMap() (either new clicks will cancel the previous request or will be totally ignored respectively). Here is one way:

doStuff() {
  return this.appUserService.getAppUser(true).pipe(
    switchMap(appUser => this.accountApi.getLoginKey(appUser).pipe(
      map(loginKey => { appUser, loginKey }),
    )),
    tap(({ appUser: /* appUser type */, loginKey: /* loginKey type */ }) => {
      this.browser = this.inAppBrowser.create(appUser.Url + loginKey, '_system');
    }),
    
    // OPTIONAL: add this take(1) to make sure Observable completes after first emission
    // if that is the desired behaviour (one way to prevent leaks)
    take(1)
  );
}

UPDATE

Modified the code to remove the deprecated switchMap with resultSelector.

SECOND UPDATE

Corrected parentheses.

Will Alexander
  • 3,425
  • 1
  • 10
  • 17
  • The behaviour I am looking for is, someone clicks button where the click event is "doStuff()", this then runs the code and opens the "InAppBrowser" which is effectively a new window that will open in the Application, therefore, clicking the button multiple times should not try and run the code more than once (if anything it would be best if only the first instance ran to save it keep calling the API uneccessarily). If they closed the popup window and then clicked the button again I would want the code to run again. Would adding take(1) achieve this and mean I do not have to unsubscribe? – WebFletch Jun 28 '20 at 18:44
  • Also, I have added the code you suggested and I get a number of errors, the first one is on switchMap: switchMap is deprecated: resultSelector is no longer supported, use inner map instead. The next error is within the switchMap where is says both "appUser" and "loginKey" cannot be found. Then in "tap" it says "All destructured elements are unused". And finally again within the "create" part of the "tap" it says it cannot find name appUser and loginKey. – WebFletch Jun 28 '20 at 19:53
  • Oh I didn't realize that switchMap's resultSelector was deprecated, am editing my answer to account for this. I always preferred this way because it seems more elegant to have everything on the same level, but here we'll have to nest one level – Will Alexander Jun 29 '20 at 08:01
  • I tried the new edit you suggested, I had to make some minor modifications to add the "AppUser" type into the switchMap and I had to remove the type from within the "tap" because it was seeing them as additional parameters (not sure if brackets are needed maybe?). However there are still a couple of errors with the code you suggested, I have edited my original question to show what I now have and what errors are still showing. Any idea what could be causing these? – WebFletch Jun 29 '20 at 09:28
  • Yep, my pipe is in the wrong place (problem with writing code directly into the browser!) Will change now – Will Alexander Jun 30 '20 at 16:42
  • The version I have used in "Edit 2" of my original question is slightly different to your version (I have the "tap" within the "switchMap", rather than using "map"). Are there any advantages/disadvantages of doing it my way or your way that you know of or are they pretty much the same? – WebFletch Jul 01 '20 at 18:06
  • 1
    I like your solution as well, I guess I just like having as much as possible happen on the top level of my Observable pipe ^^ adding error handling to our different solutions would lead to different approaches, but I think both are equally viable. Good work! – Will Alexander Jul 02 '20 at 06:29
  • Okay great, I like the idea of trying to keep as much as possible on the top level, I am slowly working my way through my project rewriting all observables so they no longer use nested subscribes and include either takeUntil or take(1) – WebFletch Jul 02 '20 at 22:36
  • Don’t forget my favourite option: subscribing in the template using the async pipe! – Will Alexander Jul 04 '20 at 06:02
  • That is not something I have used before, most of my subscriptions are either within ngOnInit (i.e. loading the initial data for the view) or on button click events (i.e. calling an API to do something). – WebFletch Jul 05 '20 at 19:02
1

There are two questions here,

  1. How to get around the nested subscription situation? @Will Alexander has answered that question. Here are some of the references that may help you
    https://angular-checklist.io/default/checklist/rxjs/Z1eFwa9
    https://www.thinktecture.com/en/angular/rxjs-antipattern-1-nested-subs/

  2. How to unsubscribe gracefully?
    a) You can unsubscribe manually from each of your subscriptions (this can get messy real fast)
    b) If you no longer need doStuff() stream, then you can use the take(1) as explained
    c) Angular recommends using async pipe wherever possible, so that the framework does the subscribe and unsubscribe part
    d) Use the takeUntil operator to unsubscribe from all you subscriptions (this comes in handy when you have many subscriptions that are not managed using async pipe) https://medium.com/@benlesh/rxjs-dont-unsubscribe-6753ed4fda87

Dwaraka
  • 179
  • 8
-1

There is no "correct way", the code you have is already good. If you want to improve it a little, you could use rxjs (switchMap for example).

Regarding the unsubscribe part, it's always better and safer to unsubscribe. With Angular, you can use the async pipe in the HTML or simply unsubscribe in the destroy fase of the component.

Diogo Brito
  • 147
  • 5
  • I don't think this is a helpful answer. There are plenty of simple suggestions we can make. – Will Alexander Jun 28 '20 at 12:53
  • Regardless, It answers both of his questions, but fair enough, hope someone do it better then – Diogo Brito Jun 28 '20 at 12:58
  • 1
    The OP's code isn't already good. It breaks a very basic norm, and that's why they've come here for help. As for the unsubscribe comment, it's not "always better and safer" to unsubscribe. There are plenty of situations where it is unnecessary, and makes a codebase far less tidy. I mean no offence at all here, simply to point out that answers like this aren't helpful and I think should be avoided. – Will Alexander Jun 28 '20 at 13:04
  • No problem, i've started recently to start help others here, thanks! – Diogo Brito Jun 29 '20 at 21:11