0

Consider a BehaviorSubject that is holding an array of objects. This subject is supposed to be subscribed in multiple places at once i.e multiple subscriptions to same subject. It receives data from backend via polling. Since polling same api can result in same data, I'm comparing the new response from backend with existing data of Subject using BehaviorSubject.value as shown in this demo below

https://stackblitz.com/edit/9ih9mv?file=index.ts

My teammates saw Ben Leah's comment on this thread How to get value from Subject

Based on @BenLesh's answer on this thread, my team is highly discouraging the use of .value or .getValue() for data comparison. If I have a service whose subject is initialised when class is loaded and is unsubscribed when class is destroyed (ngOnDestroy), is there any problem in using .value to compare data before calling .next()?

FYI, I try not to use distinctUntilChanged because if I have 1 subject subscribed to multiple subscriptions, it will trigger comparison multiple times (I.e. 1 time for each subscription).

Aayush Kumar
  • 37
  • 1
  • 8

2 Answers2

2

The crux of Ben Lesh's answer is that using .value is a sign that you're not using RxJS to it's best ability.

If you're using getValue() you're doing something imperative in declarative paradigm.

To a lesser extent, that's true with Subjects in general. They're typically used for either of two purposes. Multicasting, or bridging between imperative and declarative code.

All you need here is the multi-casting component. In most cases, you can use a operator (they use subjects under the hood) to do that for you.

A lot of your song and dance here is to implement distinctUntilChanged declaratively. In so doing, you have created a version that is both much slower (shouldn't matter here) and much harder to maintain (should matter here).

Here is how I might refactor your code (using shareReplay & distinctUntilChanged) to be a bit more in line with dogmatic RxJS.


interface Something {
  length: number
}

class SomeService implements OnInit, OnDestroy {

  /* Errors are "false", Data without a length is "undefined", and 
    everything else is "something". I wouldn't reccomend this,
    but as an example, sure.
  */
  private dataOb$: Observable<(Something | Boolean | undefined)[]>
  private pollingSubscription: Subscription;

  constructor(private readonly httpClient: HttpClient) {

  }

  ngOnInit() {
    this.dataOb$ = timer(0,1000).pipe(
      concatMap(() => this.getDataFromBackend()),
      distinctUntilChanged(Lodash.isEqual),
      shareReplay(1) // multicasting
    )

    // This service is effectively "using" itself. This means
    // the polling continues even if nobody else is listening.
    this.pollingSubscription = this.getDataObs().subscribe()
  }

  private getDataFromBackend(): Observable<(Something | Boolean | undefined)[]> {
    // This is a bizzaar function, but I assume it's just as an example
    return this.httpClient.get(url, options).pipe(
      map((response: Something[]) => {
        if (response?.length > 0) {
          return response;
        }
        return undefined;
      }),
      catchError(() => of(false))
    )
  }
  
  // I changed this from a private method
  getDataObs(): Observable<(Something | Boolean | undefined)[]> {
    return this.dataOb$
  }

  ngOnDestroy() {
    this.pollingSubscription.unsubscribe();
  }
}

A quick aside:

Array<int> is the same as int[] and
Array<int|boolean> is the same as (<int|boolean>)[]

Update

If you want to (for example) ignore errors and empty emissions:

private getDataFromBackend(): Observable<Something[]> {
  return this.httpClient.get<Something[]>(url, options).pipe(
    filter(response => response?.length > 0),
    catchError(() => EMPTY)
  )
}
Mrk Sef
  • 7,557
  • 1
  • 9
  • 21
  • Corrected my getData function. Returning undefined instead of false now. It was just a mistake. I'll try out your solution too. Can you tell me if the getData function is still bizzare? I think you pointed out because of my typo haha – Aayush Kumar Jan 13 '22 at 17:11
  • @AayushKumar Normally, `undefined` is not supposed to have semantics. In your case, you've imbued it with "the back-end emitted an empty array or an error" semantics. I would never do that in production as it will surely get confusing/messy later on. For a small example it's fine, but even then I'd prefer `null`. In production you should model your data explicitly. For example: `catchError(() => EMPTY)` will ignore your errors. `filter(res => res?.length > 0)` will ignore empty arrays from the source. If you don't want to ignore them, put them into a data structure you can consume later. – Mrk Sef Jan 13 '22 at 17:38
  • It's working somehow! I have two questions. 1) I can see that it's comparing previous responses with new ones. I'm storing the currentResponse as global variable and not passing that field for comparison. So where does dintinctUntilChanged get the previous resonse from? Does it keep it cached? I thought it would take the previous response from source observable (timer in this case). 2) if you see console logs, previous response is being pushed instead of current response. Can you help me correct it? https://stackblitz.com/edit/9ih9mv?file=index.ts – Aayush Kumar Jan 13 '22 at 18:31
  • @AayushKumar It doesn't return values from the timer, it returns the random numbers your generating (which is always a 0 or a 1). Also, why shove a BehaviorSubject in there? It's not doing anything – Mrk Sef Jan 13 '22 at 18:31
  • Yeah found that. My bad. Can you check the demo again? Corrected and updated. This time it's coded the way I wrote a few of my services. – Aayush Kumar Jan 13 '22 at 18:36
  • @AayushKumar To answer your question: 1) `concatMap` is turning timer emissions into concatenated `getDataFromBackend` calls the source is these concatenated calls, not the timer. 2) You don't push data, you just push dataStatus. You push dataStatus before you set the service data so ofc it will not be the new one until after. 3) You're trying super hard to make your RxJS code as imperative as possible which is making it a lot buggier than it needs to be :) Remove that behaviourSubject, combine your dataStatus and Data into one, stop worrying about the order of what's set when, where, & how :) – Mrk Sef Jan 13 '22 at 18:48
  • Can you help me understand below things by modifying my demo? 1) how to push current value instead of previous? 2) how to combine data and data status into one? 3) if I remove data status, how will my components know if data is unavailable with 200 http code or there's an http error? 4) what do you mean by imperative? – Aayush Kumar Jan 13 '22 at 18:54
  • @AayushKumar 1) switch those two lines, then it'll work `this.dataStatus.next(DataStatus.Available); this.data$ = data;` . To have better bug-free code with RxJS, you can read articles on how other people use it :) – Mrk Sef Jan 13 '22 at 19:05
0

You can use pairwise() operator and compare the two values

documentation: https://rxjs.dev/api/operators/pairwise

concatMap(() => this.getDataFromBackend()),
pairwise(),
switchMap(([oldResponse, newResponse]) => {
    // ...logic
    if (!Lodash.isEqual(oldResponse, newResponse)) {
        this.subject.next(newResponse);
    }
})

demo: https://stackblitz.com/edit/jmumnz-amtlq4?file=index.ts

Danail Videv
  • 763
  • 6
  • 16