1
\$\begingroup\$

I load an array from an API that is available throughout the user's whole session. This data is unlikely to change, so it can be cached safely. I have a DataService that works like this:

  • get data from the class property (scenario: going to a new route)
  • if nothing is there, get data from local storage (scenario: reload)
  • if nothing is there, fetch data from API (scenario: new session)

This is what I've come up with:

import { Injectable } from '@angular/core';
import { HttpClient } from '@angular/common/http';
import { Observable, of } from 'rxjs';
import { map, catchError, retry } from 'rxjs/operators';

@Injectable({
  providedIn: 'root',
})

export class DataService {
  protected data: Array<any> = null;

  constructor(private http: HttpClient) { }

  public getData(): Observable<Array<any>> {
    if (this.data === null) {
      this.data = JSON.parse(localStorage.getItem('data'));
  }

  if (this.datas !== null) {
    return of(this.data);
  }

  return this.http.get<Object>(this.url + this.route).pipe(
    retry(3),
    map(response => {
      this.data = response as Array<any>;
      localStorage.setItem('data', JSON.stringify(this.data));
      return this.data;
    }),
    catchError(error => {
      this.handleError(error);
      return [];
    })
  );
}

The service is registered in the app.component.ts and used like this in other components:

import { Component, OnInit } from '@angular/core';
import { DataService } from 'src/app/service/data.service';
import { Subscription } from 'rxjs';

@Component({
  selector: 'app-example',
  templateUrl: './example.component.html',
  styleUrls: ['./example.component.less'],
  providers: []
})

export class ExampleComponent implements OnInit {
  protected subscribers: Subscription[] = [];

  public data: Array<any> = [];

  constructor(public data: DataService) { }

  ngOnInit() {
    this.subscribers.push(
      this.data.getData().subscribe((data) => { this.data = data })
    );
  }

  ngOnDestroy() {
    for (let subscriber of this.subscribers) {
      subscriber.unsubscribe();
    }
  }
}

Is it a good approach? Is this "the Angular way"? Can it be improved? Is it readable? Does this indeed increase performance? At least it uses fewer HTTP requests.

This class is shortened a bit for the review, so don't worry that there's no handleError method.

\$\endgroup\$
  • 1
    \$\begingroup\$ This class is shortened a bit for the review - I hope you have a good reason for doing this because we usually don't like shortened code ;-] \$\endgroup\$ – t3chb0t Sep 24 '18 at 18:34
  • \$\begingroup\$ @t3chb0t I removed only the handleError method, that currently logs to the console and the assignment of this.route and this.url which comes from the environment. Should I add it? I removed it, because I thought it's not relevant for the review. \$\endgroup\$ – lampshade Sep 25 '18 at 10:08
  • 1
    \$\begingroup\$ mhmm it looks like it's not such a big deal, this time I'll forgive you ;-) \$\endgroup\$ – t3chb0t Sep 25 '18 at 10:12
1
+50
\$\begingroup\$

If you have configured caching headers (both on request and response) correctly, the browser will do caching for you. You can have your code just do the request and the browser will decide whether to pull data from the server or use a cached response. You may need to look up on what your library (i.e. Angular's HttpClient) defaults to doing and/or inspect your requests and responses to see if caching is done correctly. Some libraries may disable caching entirely or may require you to explicitly tell it to consider cached results.

If you want to manually cache requests, you might want to look at Angular's HttpInterceptor class. It works in tandem with HttpClient APIs and allows you to write code that sits in between the caller (your code) and the target (the server), allowing you to modify both request and response. You can use this as an intermediary to cache responses as well as cancel requests and return cached responses. One advantage of this approach is you don't have to write a special class that your consumer must know and use. You simply drop in the interceptor and use HttpClient APIs as is.

\$\endgroup\$
1
\$\begingroup\$

Short review

  if (this.datas !== null) {
    return of(this.data);
  }

That looks like a bug, should it not be (this.data !== null) ?

Also, what is of, where is it defined?

Other than, the code is very clean in both parts. So clean, that I almost would switch the of and the localStorage call. But since this would dupe the null check, that might in the end not be the best idea.

  if (this.data !== null) {
    return of(this.data);
  } else {
      this.data = JSON.parse(localStorage.getItem('data'));
      if (this.data !== null) {
        return of(this.data);
      }
  }
\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback. Good catch - luckily it was a just a type, while creating the question. of is imported here: import { Observable, of } from 'rxjs'; - it's part of rxjs. \$\endgroup\$ – lampshade Oct 2 '18 at 7:53

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.