OK, maybe this would be less error-prone if we did this instead:
fetchAllProducts(): Observable<Product[]> {
return this.http.get<Product[]>(API_URL).pipe(
share(),
tap(prods => this.allProducts$.next(prods));
}
That should make it harder to accidentally leak subscriptions, but keep the same basic behavior of “whoever calls fetchAllProducts
, it causes all subscribers to watchAllProducts
to receive the result”.
A few notes:
- you shouldn’t need to be passing
id
here, it would typically be included in theProduct
. The fewer arguments you have to a function (I do my best to avoid having more than one, if I can), the less you have to worry about putting them in the proper order. - do everything in your power to avoid casts (
res as Product
), and you really shouldn’t need one here, because… PUT
requests shouldn’t be returning anything, IMHO. I find following this rule helps me avoid accidentally breaking the idempotance part of the fundamental contract ofPUT
on the server side: you should be able to make the samePUT
request 1, 5, or 1000 times in a row and end up in the same place.- I would not call
fetchAllProducts
inupdateProduct
, because it’s too heavy. Instead, I would manually walk throughallProducts$.value
(or keep it in a hash instead of an array) and update the local notion ofallProducts$
vianext
.
So, I would do something more like this:
updateProduct(newb: Product): Observable<Product> {
return this.http.put(URL).pipe(map() => {
let allprods = clone(this.allProducts$.value) || [];
let nprods = allprods.length;
let foundit = false;
for (let i = 0; i < nprods; ++i) {
if (allprods[i].id === newb.id) {
allprods.splice(i, 1, newb);
foundit = true;
break;
}
}
if (!foundit) {
allprods.push(newb);
}
this.allProducts$.next(allprods);
return newb;
});
}