Is it ok to include logic inside an Observable subscribe method?

Question

I was watching @john_lindquist's egghead tutorial on RxJS and he hammered the point about not including busienss logic instead of a .subscribe() method.

So I'm creating a canActivate guard to protect against users going to an invalid route, and I can't help but build logic into the subscribe method. Is there a better way to do this?

import { Injectable } from '@angular/core';
import { CanActivate, ActivatedRouteSnapshot, Router } from '@angular/router';
import { Subscription } from 'rxjs/Subscription';
import { Subject } from 'rxjs/Subject';

import { Apollo, ApolloQueryObservable } from 'apollo-angular';

import { GQTicketId } from './ticket.model';



@Injectable()
export class TicketRouteActivatorService implements CanActivate {

  public ticket;
  private returnTrue: Subject<Boolean> = new Subject<Boolean>();
  private ticketSubscription: Subscription;

  constructor(private apollo: Apollo, 
              private router: Router) { }

  canActivate(route: ActivatedRouteSnapshot ) {

    this.ticketSubscription = this.apollo.watchQuery({
      query: GQTicketId,
      variables: {
        _id: +route.params['id']
      }
    }).subscribe(({data})=>{
      this.ticket = data['ticket'];

      // this doesn't seem right, atleast based on the guidance from John Linquidst. Should this logic be "Rx'd" some how?
      if(!this.ticket) {
          this.router.navigate(['provision/requests/error/404']);
      } else {
        this.returnTrue.next(true);
      }

    }
    );

      if (this.returnTrue) return true;
  }

  // do we need to unsubscribe?
  ngOnDestroy():void {
      this.ticketSubscription.unsubscribe();
  }

}

_"I was watching @john_lindquist's egghead tutorial on RxJS and he hammered the point about not including busienss logic instead of a .subscribe() method."_ - What was his reasoning? — Paul Samsotha, Apr 12 '17 at 00:20
One major problem with the implementation in the snippet is that it assumes the `subscribe` is synchronous. You should return the observable from `canActivate` and let the router implementation subscribe to it (as in the answer below). `canActivate` can return a boolean, a promise or an observable. — cartant, Apr 12 '17 at 02:14

snorkpete · Accepted Answer · 2017-04-12T08:54:03.790

To answer your question, there is a more 'RxJS-friendly' way to do what you want. ("Better" is a matter of opinion). So, for eg, if I were implementing this, I would do it this way:

canActivate(route: ActivatedRouteSnapshot) {

  // canActivate can return an Observable - 
  // the Observable emitting a true or false value
  // is similar to returning an explicit true or false
  // but by returning the observable, you let the router
  // manage the subscription (and unsubscription) instead of doing it yourself
  // doing this removes the need for the `unsubscribe` in your code
  return this.apollo.watchQuery({
    query: GQTicketId,
    variables: {
      _id: +route.params['id']
    }
  })
  // there should be no arguing about this point -
  // you only care about the data['ticket'] property,
  // not the entire data object, so let's map our observable
  // to only deal with ticket
  .map(response => response.data.ticket)
  // you want to do a side effect if ticket is falsy
  // so the do operator is your best friend for side effect code
  .do(ticket => {
    if(!ticket) {
      this.router.navigate(['provision/requests/error/404']);
    }
  })
}

Without the comments:

canActivate(route: ActivatedRouteSnapshot) {

  return this.apollo.watchQuery({
    query: GQTicketId,
    variables: {
      _id: +route.params['id']
    }
  }).map(response => response.data.ticket)
    .do(ticket => {
        if(!ticket) {
          this.router.navigate(['provision/requests/error/404']);
        }
      })
    }

I watched John's videos but can't remember the specifics at this time. But, the key takeaway with RxJS is that with the proper use of the operators, you can usually remove a lot of imperative code that would otherwise end up in your subscribe method. That doesn't mean that using subscribe is bad in and of itself - it's just that having logic in subscribe is usually an indicator that you're not using RxJS to its best potential.

`do` operator is renamed to `tap` in RxJS 5.5. see https://stackoverflow.com/questions/48025965/how-would-i-use-do-as-an-rxjs-lettable-operator — illnr, Dec 29 '18 at 22:46

Is it ok to include logic inside an Observable subscribe method?

1 Answers1