2
\$\begingroup\$

This is just a dummy example, but in my real world application I get a lot of data from the web API and I'd like to manipulate it on the client's side. So, I kind of stored the observable I get from the HTTP request locally and change the values with map:

@Injectable()
export class UsersApiService {

  private readonly baseUrl: string = 'https://reqres.in/api/users';
  resource$: Observable<any>;

  constructor(private http: HttpClient) {
    this.resource$ = this.http.get<IUserDetails[]>(this.baseUrl).pipe(
      tap((data) => {
        console.log('"getUsers" successfully called!');
      }),
      map((data: any) => {
        return data.data;
      })
    ).publishReplay(1).refCount();
  }

  getUsers(): Observable<IUser[]> {
    return this.resource$.pipe(
      map((data: IUserDetails[]) => {
        return <IUser[]>data.map((u) => {
          return {
            id: u.id,
            name: `${u.first_name} ${u.last_name}`
          };
        });
      })
    );
  }

  getUserById(id: number): Observable<IUserDetails> {
    return this.resource$.pipe(
      map((data) => {
        return <IUserDetails>data.find(x => x.id === id);
      })
    );
  }

}

Here's an example plunker

Is this the correct approach at reusing an observable? What would be better? Am I creating any memory leaks and what are the general drawbacks of my approach? Thanks!

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

There's nothing (obviously) wrong with your code. You deal with observables the efficient way, IMO. Cache is good. The fact you're constructing a "base" observable in constructor without subscribing to it there is good as well...

There are only a few things I can point out:

  • Design: Be careful with caching, if your user information changes often, the cache may bite you back. You may want to implement some kind of cache invalidation policy.
  • Design: If your system has thousands of users, you may want to not load all of them into memory at once. That would require your API support pagination though. If the users are few, it's totally okay to do what you're doing, I think.
  • RxJs: not sure why pipe() and tap() are used the way are used. I think, it's easier to use do() and map() as shown in the code below. Chaining looks much more readable (as "step-by-step" explanation of how the data is transformed into the desired result).
  • Idioms: TypeScript is all about types. Do specify a known type when possible instead of using any. E.g. Observable<IUserDetails[]> is better than Observable<any[]>.
  • Style: Variable naming is important. Do not use one-letter names (u), they are pure evil. Names like data are also evil, in your case it's better to call it response. The resource$ can probably be nicer to the reader if called userDetails$. You get the idea.

@Injectable()
export class UsersApiService {

  private readonly baseUrl: string = 'https://reqres.in/api/users';
  userDetails$: Observable<IUserDetails[]>;

  constructor(private http: HttpClient) {
    this.userDetails$ = this.http
      .get<IUserDetails[]>(this.baseUrl)
      .do(response => console.log('"getUsers" successfully called!'))
      .map(response => response.data)
      .publishReplay(1)
      .refCount();
  }

  getUsers(): Observable<IUser[]> {
    return this.userDetails$
      .map(userDetailsList =>
        userDetailsList.map(userDetails =>
          <IUser>{ id: userDetails.id, name: `${userDetails.first_name} ${userDetails.last_name}` }
        )
      );
  }

  getUserById(id: number): Observable<IUserDetails> {
    return this.userDetails$
      .map(userDetailsList => <IUserDetails>userDetailsList.find(userDetails => userDetails.id === id));
  }
}
\$\endgroup\$
  • \$\begingroup\$ Sadly I can't comment on the answer above, so I'll just write a reply here. Thanks for your time! Firstly, I'm using this for a menu, since it has a lot of itmes (>1000), so I'll just manipulate the data locally, not going to query server each time the user clicks a link. I'm using pipe since I believe Google has been forcing that with their new version of HttpClient. Other two comments I agree with, will improve that part of the code. Now, I have a follow up question: do I really need .publishReplay(1).refCount(); in the initial HTTP request? \$\endgroup\$ – uglycode Dec 13 '17 at 22:56
  • \$\begingroup\$ Could you show sources where Google forces for pipe? I am unsure about it. \$\endgroup\$ – Igor Soloydenko Dec 13 '17 at 22:58
  • \$\begingroup\$ .publishReplay(1).refCount(); is basically a "standard" simple implementation of a cache. Without it, each subscription on getUserById()/getUsers() will result in an HTTP call which you want to avoid. So, yes you need this code as long as you want to cache the result. \$\endgroup\$ – Igor Soloydenko Dec 13 '17 at 23:00
  • \$\begingroup\$ @user155748 do you have two accounts - this one and user155724? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 13 '17 at 23:02
  • 1
    \$\begingroup\$ Thanks for your time! Firstly, I'm using this for a menu, since it has a lot of itmes (>1000), so I'll just manipulate the data locally, not going to query server each time the user clicks a link. Here's the part: angular.io/tutorial/toh-pt6 search for .pipe( I believe there are 17 occurrences. I've just tested the application without publishReplay and indeed, the service gets called each time. Hm, hm, I wonder why is that, without this publishReplay, we have a cold observable? \$\endgroup\$ – uglycode Dec 13 '17 at 23:10

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.