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);
})
);
}
}
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!
2 Answers 2
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()
andtap()
are used the way are used. I think, it's easier to usedo()
andmap()
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 thanObservable<any[]>
. - Style: Variable naming is important. Do not use one-letter names (
u
), they are pure evil. Names likedata
are also evil, in your case it's better to call itresponse
. Theresource$
can probably be nicer to the reader if calleduserDetails$
. 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));
}
}
-
\$\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 ofHttpClient
. 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– uglycode2017年12月13日 22:56:34 +00:00Commented Dec 13, 2017 at 22:56 -
\$\begingroup\$ Could you show sources where Google forces for
pipe
? I am unsure about it. \$\endgroup\$Igor Soloydenko– Igor Soloydenko2017年12月13日 22:58:08 +00:00Commented Dec 13, 2017 at 22:58 -
\$\begingroup\$
.publishReplay(1).refCount();
is basically a "standard" simple implementation of a cache. Without it, each subscription ongetUserById()
/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– Igor Soloydenko2017年12月13日 23:00:20 +00:00Commented Dec 13, 2017 at 23:00 -
\$\begingroup\$ @user155748 do you have two accounts - this one and user155724? \$\endgroup\$2017年12月13日 23:02:12 +00:00Commented Dec 13, 2017 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– uglycode2017年12月13日 23:10:33 +00:00Commented Dec 13, 2017 at 23:10
It looks good, you can also use ReplaySubject
to minimize a number of rxjs operators:
private _users$ = new ReplaySubject<IUserDetails[]>();
...
this.http.get<IUserDetails[]>(this.baseUrl)
.pipe(
tap(() => console.log('"getUsers" successfully called!')),
map(({ data } : IUsersResponse): IUserDetails[] => data)
)
.subscribe((users: IUserDetails[]) => {
this._users$.next(users);
});
I prepared an example: https://stackblitz.com/edit/angular-eemkbb
Explore related questions
See similar questions with these tags.