2
\$\begingroup\$

I have a need to store data retrieved my ASP.NET Web Api to stop making redundant API calls for the same data when I navigate through my Angular page. On my API I have a database which stores a list of printers known to the PC and stores some basic information about them. Here is the model:

export class Printer
{
    id: number;
    name: string;
    active: boolean;
    blacklisted: boolean;
    default: boolean;
}

This printer data is consumed by several components and as I previously had it each time I needed this data I had to request it from my backend. Here is my attempt reducing redundant API calls:

import { Injectable } from '@angular/core';
import { Printer } from '../models/printer';
import { HttpClient } from '@angular/common/http';
import { Observable } from 'rxjs/Rx';
import { catchError, tap } from 'rxjs/operators';
import { of } from 'rxjs/observable/of';

@Injectable()
export class PrintersService {
  private static readonly apiUrl = "/api/printers";
  private static printers: Observable<Printer[]> = undefined;

  constructor(private http: HttpClient) { }

  getPrinters(refresh: boolean = false): Observable<Printer[]> {
    if (refresh || PrintersService.printers === undefined) {
      this.log("Observable<Printer[]> is undefined or the method requested to refresh");
      this.apiGET();
    } else {
      this.log("Observable<Printer[]> is cached, no API call will be made");
    }

    return PrintersService.printers;
  }

  /*
    Not sure how to get this method to work, I need to get a single 
    observable object from my observable array of objects.  For now I'm 
    just having everyone retrieve the list and then just search that list 
    manually, but it would be really nice if I could just make a call for 
    a single object instead of getting the whole mess.   

    getPrinter(id: number, refresh: boolean = false): Observable<Printer> {    
    if (refresh || PrintersService.printers === undefined) {
      this.apiGET();
    }

    var result: Observable<Printer> = of(null);
    PrintersService.printers.subscribe((printers) => {
      var target = printers.find(p => p.id === id);
      result = of(target);
    });

    return result; 
  }*/

  updatePrinter(printer: Printer) {     
   PrintersService.printers.subscribe(printers => {
      printers.forEach((element, index) => {
        if (element.id === printer.id) {
          printers[index] = printer;
          this.http.put(PrintersService.apiUrl, printer).pipe(
            tap(_ => this.log(`Updated printer id=${printer.id}`)),
            catchError(this.handleError<any>('updatePrinter'))
          );
        }
      });
    });
  }

  private apiGET(): boolean {    
    this.log("GET: " + PrintersService.apiUrl);
    PrintersService.printers = this.http.get<Printer[]>(PrintersService.apiUrl);
    return PrintersService.printers === undefined;
  }

  private handleError<T> (operation = 'operation', result?: T) {
    return (error: any): Observable<T> => {
      console.error(error);
      this.log(`${operation} failed: ${error.message}`);
      return of(result as T);
    }
  }

  private log(message: string) {
    console.log('PrintersService: ' + message);
  }
}

From my initial testing with this service, it seems to work well. API calls are being made only when the data needs to be retrieved. My main concern with all of this is there is a more appropriate way to do this in Angular. I'm new to front-end stuff, let alone Angular and I don't know what best practices are for this particular scenario. This is how I'd handle it if I were writing a desktop app, granted I definitely need something in there that causes the data to refresh every now and then.

\$\endgroup\$
2
\$\begingroup\$

"Caching" an Observable

PrintersService is by default a singleton in Angular just like any other service (This is thanks to the way dependency injection container works in Angular). This means you don't need to have the class fields as static. Make them instance fields instead:

private readonly apiUrl = "/api/printers";
private printers: Observable<Printer[]> = undefined;

updatePrinter(printer: Printer) {     
  this.printers.subscribe(printers => {
    printers.forEach((element, index) => {
      if (element.id === printer.id) {
        printers[index] = printer;
        this.http.put(this.apiUrl, printer).pipe(
          tap(_ => this.log(`Updated printer id=${printer.id}`)),
          catchError(this.handleError<any>('updatePrinter'))
        );
      }
    });
  });
}

private apiGET(): boolean {    
  this.log("GET: " + this.apiUrl);
  this.printers = this.http.get<Printer[]>(PrintersService.apiUrl);
  return PrintersService.printers === undefined;
}

Caching Pattern

I commonly see the following pattern for "caching" observables in various code bases:

const CACHE_SIZE = 1;

this.cache$ = this.requestJokes().pipe(
  shareReplay(CACHE_SIZE)
);

It will not very nicely with the code you have because the pattern addresses the GET calls assuming there's no mutation to the list of Jokes (Printers in your case) such as PUTs, POSTs or DELETEs.

Alternative 1: Observable as BehaviorSubject

You can use a BehaviorSubject<Printer[]> to deal with the state manually. BehaviorSubject is an observable and an observer at the same time. It has methods next(newValue) and getValue() which make manual state control possible.

Alternative 2: More advanced state management

I've been where you are many times. The complexity of manual observable/behaviour subject led me to a conclusion the approach is not scaling well. It holds me back from actually progressing on feature work itself.

My recommendation is to look at a state management library like @ngrx/store. The library ideas revolve around a few concepts one which is Store. The store represents the state of your application that is exposed as an observable. The operations that change (update) the stored are written in a special way (via so called "reducers").

In my experience, the apps written with @NgRx are simpler in building and even simpler in maintenance. You'll need invest some time in picking up the library's approach, API, and so on but the overall entry barrier is relatively low.


Side notes

  • Use interface instead of class if possible.
  • Do not use keywords as field names if possible, (i.e. default).
  • Use is* or are* in member names to denote the boolean nature of the member. (I've seen word "default" used as a verb with the meaning of reset to default(s)).

export interface Printer
{
  id: number;
  name: string;
  isActive: boolean;
  isBlacklisted: boolean;
  isDefault: boolean;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the input. I'm curious, why should objects be interfaces instead of classes? \$\endgroup\$ – user138741 Apr 9 '18 at 19:47
  • 1
    \$\begingroup\$ @user138741 the interfaces imply only the "surface" of the objects. In your case it's important because Printer is a DTO (data transfer object). When you receive such object from an http call it will not have any methods on it even if class Printer defines some. An interface would relay that fact better. There are more reasons but they're more subtle, so I'll not cover them here. \$\endgroup\$ – Igor Soloydenko Apr 9 '18 at 19:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.