Best practice: Nested subscriptions to receive user data?

Ionic Framework ionic-v3
1

I’m using Firebase as backend. With Firebase, some authentication data is stored with firebase automatically (currently logged in user’s id, email and password). The other user data (name, address, …) is stored in my Firebase database.

To get the currently logged in user’s data, I have to subscribe to Firebase’s auth methode first. Using angularfire2 I do the following:

this.af.auth.subscribe(data => {
  ... // data.uid gives me the user's id
}

Now I know the user’s id and can subscribe to my Firebase database to get the other user data by doing:

this.af.database.object('/users/'+userId).subscribe( userData => {
   ...
}

If I combine both it’s a nested subscription looking like this:

this.af.auth.subscribe(data => {
   this.af.database.object('/users/'+user.uid).subscribe( userData => {
       ...
   }
}

This works, however, it doesn’t feel right to have two nested subscriptions. How do you guys deal with this? What would be a “best practice approach” here?

2

If operation B relies on information provided by operation A, then I think nested subscriptions are necessary. If the two operations are independent, you could look into using forkJoin, which is roughly equivalent to Promise.all.

3

or even switchMap is a very good approach for such things and maybe a better way.

https://www.learnrxjs.io/operators/transformation/switchmap.html

1 Like
4

Yes, in this case operation B strongly relies on operation A. However, when I do the following:

ionViewDidLoad() {
    this.authService.getCurrentUser().subscribe( user => {
      console.log("subscription A")
      this.userService.getCurrentUsersData(user.uid).subscribe( userData => {
        console.log("subscription B")
      });
    });
  }

Everytime the page loads, the console’s result looks like this:

subscription A
subscription B
subscription B
subscription B

For now I used “take(1)” to avoid operation B to be called several times.

ionViewDidLoad() {
    this.af.auth.take(1).subscribe( user => {
      console.log("subscription A")
      this.af.database.object('/users/'+user.uid).take(1).subscribe( userData => {
        console.log("subscription B")
      });
    });
  }

This works very well. However, I’m still not sure if this is a proper approach, or if I should use anything else than subscribe. And - of course - I’m loosing the subscription, so I’m not informed of new updates, which is OK in my case. It may be a problem in other cases.

@bengtler: I never used switchMap before. Would you use it in this case or - as rapropos pointed out - only if the operations don’t rely on each other?

5

Regarding take(1):

In my auth and database services, I set up an explicit distinction between methods that provide a snapshot and methods that provide a stream. In your case, getCurrentUserSnapshot() returns either a Promise or an Observable that has been take(1)'ed, so either way you’re guaranteed to get one value and completion (or a read/write error). While getCurrentUser() or getCurrentUserStream() would emit a new value each time the current user changes.

I don’t know if that’s the pinnacle of best practice, but I think it’s a good idea to push take(1) back to the service, so if you make a “stupid” mistake, you’re only making it in one location.

6

That indeed makes sense, @AaronSterling . I will update my services to reflect that. Thank you for the hint.

7

You generally want to avoid nesting async calls, it’s a nightmare to read and makes error handling borderline impossible.

This stack overflow has a good example: http://stackoverflow.com/questions/34523338/rxjs-sequence-equvalent-to-promise-then

Ultimately you want to use flatMap, which is equivalent to Promise’s .then(), which in the promise world is how you chain instead of nest.

Edit: Or as @bengtler suggested switchMap depending. In either case avoid nesting.

8

Thank you @rloui. I tried to avoid nesting the async calls by separating them. I built an userService which receives the auth-data from the authService in it’s constructor and then offers methods to access the users data in my database. Therefore, these methods need to know the userId, which is received in the constructor.

In a minimal code example it looks like this:

user-service.ts:

@Injectable()
export class UserService {
  userId: any;

  constructor(...) { 
    this.authService.getCurrentUserStream().subscribe( user => {
      this.userId = user.uid;
    }, error => {...});
  }
  
  // several methods like this (depending on userId)
  getCurrentUsersDataOnce(){
    return this.af.database.object('/users/'+this.userId).take(1);
  }

  getCurrentUsersDataStream(){
    return this.af.database.object('/users/'+this.userId);
  }
  ...
}

This works in the test environment. However, I’m not sure if it will always work in real life, for example if the userId is not received in time in the userService’s constructor. Then the methods cannot be executed, since userId is missing.

Would you, hence, recommend to use flatMap for each method in userService?

@Injectable()
export class UserService {

  constructor(...) {  }

  getCurrentUsersDataStream(){
    return this.authService.getCurrentUserStream()
      .flatMap( user => {
      return this.af.database.object('/users/'+user.uid).take(1)
  });
  ...
}

It kind of feels like complicating it if I do this for every function in userService (about 20-30 of them in total). Is there a smarter way of “centralizing” this?

Thank all of you. I really appreciate your help!

9

Can’t you just discourage transition to a page that would call those methods until your user is logged in? Pause on the page with a loading spinner until login completes, and define your database rules so that someone can only read a user’s info if they have that user’s auth token. As long as you handle the error gracefully with e.g. a please try again message, you’re locking out the attacker while comforting the user who accidentally hit the back button.

You have to handle user security on the backend anyway.

10

Yes, sure, I can do that. I wanted to move everything to the service and separate it from the page. But I think you are right and it’s better to receive the userId on the page itself and then call the service passing over the userId. Then my service is cleaner.
Thank you!

11

Cool. Look. Something occurs to me. @rlouie’s comment is great for Observables in general, and I’ve seen him make some high-quality posts here, so he’s someone I take seriously. But I’m just not sure how well his comment matches your specific situation, which is authentication using AngularFire 2. The issue is that this.af.auth is a stream but not an rxjs Observable, and the same is true for this.af.database. For example, if you want to test whether an object exists at your this.af.database.object storage reference using the $exists property, you need to subscribe to the stream; using map, flatmap, etc., won’t reveal this. (And actually, maybe there’s a way to do this that I haven’t found. This is one of the areas of AF2 that has not yet been documented,)

AF2 is still in beta, so you might run into issues that are undocumented, or contradicted by documentation that has not yet been updated. It’s happened to me. If it happens to you, the AngularFire GitHub issues threads are your friend.

1 Like
12

Ya my comment was more of an in general best practice, I’ll admit I didn’t look super deeply into this specific use case. Just wanted to throw out the general best practice.

I’ve never used angular fire, I’ve always written my own back end code, and I did not realize those were streams and not observables. So good call out, you went deeper than I did. Sorry, hope I didn’t create more confusion! :slight_smile:

1 Like
13

Good point @AaronSterling. For the usecases I’m working on right now (receiving userId and then fetching some data from the database) flatMap works just fine. But you are right, in other cases I will have to subscribe to the af-Observables.

Talking about flatMap: I tried to implement it, as suggested by @rloui. And I’m pretty happy with it. However, I’m not sure how to implement an IF case within the flatMap statements. Let me explain in a code example:

functionA()
    .flatMap( data => {
      if (data === true) {
        // continue with functionB
        return functionB()
      } else {
        // STOP HERE
      }
    })

     // everything from here on is only to be called if the case is true.  
    .flatMap( data2 => {
      return functionC(data2)
    })
      
    .subscribe( (data3) => {
      console.log(data3);
    });

If the case is true, the next function should be called. Else, I want the action to stop. No need to call the other functions.

I thought about just leaving the else-statement empty, but this (of course) doesn’t work. :wink:

14

I just set up a firebase account. I’m looking into it just for my own knowledge right now. If I have a revelation I’ll share it :slight_smile:

It does look like you’ve done a nice job here of flattening your nesting, so it feels like you’re moving in the right direction. Can you clarify the reason you want to break out there? You could do Rx.Observable.throw(new Error('No Data Found')), which should break the chain and send the code to any catch block. https://xgrommx.github.io/rx-book/content/getting_started_with_rxjs/creating_and_querying_observable_sequences/error_handling.html

15

I’m curious about what you are going to say about firebase :wink:

Regarding why I want to break out there: My users can decide whether their “status” (let’s call it that for simplicity reasons) is set automatically or set by themselves. The nested code runs every minute using setInterval(). If the user decided to set the status manuallly, it’s supposed to stop right there (no need to execute all other functions, since the status is already set buy the user. And: No error at all, but common case). However, if the user wants the status to be set automatically, all the other functions need to be called one after the other to decide how to set the status.

Would you still do that by throwing an error?

1 Like
16

Okay, setup firebase and authentication. I actually think my initial idea was right on, and it seems like you’ve implemented it well. Here’s what I came up with:

this.af.auth
  .distinct((x, y) => x.uid === y.uid)
  .flatMap(auth => {
    return this.af.database.object('/users/'+ auth.uid);
  })
  .subscribe(user => console.log(user.address));

Just logging a user’s address to the console, but this totally works and has zero nesting, just a flat chain. You could of course chain as much together as you need. I also used distinct and only grab distinct uids, rather than just taking 1. Maybe overkill (or maybe even totally unnecessary) in this case but just take(1) to me seemed iffy. Again only just jumping in to firebase though.

Regarding breaking out to let the user change their status, and having it run every minute seems pretty strange to me as well, so unfortunately I’m not exactly sure in that case. You certainly could do it by throwing an error, then catching that and doing something else. It’s just tough for me to know here without the specifics like how the status is set, why it needs to run every minute, etc.

17

Also, just noticed in your code you should avoid creating inline functions just to call named functions. Passing the named functions directly is way easier to read:

So change

.flatMap( data2 => {
  return functionC(data2)
})      
.subscribe( (data3) => {
  console.log(data3);
});

To:

.flatMap(functionC)      
.subscribe(console.log);

This let’s you write really nice and readable code if your functions are named well. Like if I pulled my example inline functions out into individual functions it could be:

this.af.auth
  .distinct(compareUIDs)
  .flatMap(getUserByUID)
  .subscribe(logUserAddress)

For one-liners I usually inline, but anything more I like to pull out into named functions to make everything really nice and readable.

18

Interesting to see that you used distinct for this… I have to take a deeper look into this tomorrow. And especially thank you for the second advice regarding inline functions. Didn’t know that it works this way. Definately looks much better!!

Okay, I didn’t want to scare you away you with too much information, but maybe it’s necessary for you to fully understand what I want to do. I described this in this topic. Hope this helps to better understand where I want to go? If not, please say so, I can try to explain in other words…

To be honest it still feels a little weird to throw an error here to break out…

19

Ya I can try to take a look, and regarding distinct I didn’t look that deeply into it. Wasn’t sure if it would like emit two users if you logged out then logged back in as someone else or what. As I put in my description it might be totally unnecessary. Just felt like a better way to make sure it wasn’t called twice, but again, at time of writing I’d spent 50 minutes just setting up firebase and messing around, so…maybe not needed.

With the inline functions, ya, if you think about it, those functions take a function as the argument. By creating an arrow function then you are defining inline the function you pass as the argument. There’s no reason to define a function just to call another one, so you just pass in the function you want called.

20

Could you give an example of this? I’m a firebase noob so I’m not fully following…