Code Review - Chat using Async pipe

[zealous nacelle]

Let's say I have a Chat, I load messages from Rest and then I listen fron new messages from WebSocket.
I also need the ability to clear all messages. Here is the simplified HTML template:

<div *ngFor="let message of messages$ | async"> {{message}} </div>
<button (click)="clearChat">Clear Chat </button>

and the simplified component code:

public messages$: Observable<string[]>;
private clearChatSubject = new BehaviorSubject<boolean>(false);

ngOnInit() {
  const initialMessages$: Observable<string[]> = this.chatRestService.getMessages$(this._chatRoomId);

  const newMessages$: Observable<string[]> = this.chatSocketService.getMessageEvents$(this._chatRoomId).pipe(
    startWith([])
  );

  this.messages$ = initialMessages$.pipe(
    concatMap(initialMessages => {
      return this.clearChatSubject.asObservable().pipe(map(reset => reset ? [] : initialMessages))
    }),
    switchMap(initialMessages => {
      return newMessages$.pipe(
        map(newMessages => initialMessages.concat(newMessages)),
        tap(mergedMessages => initialMessages = mergedMessages)
      )
    })
  );

}

public clearChat() {
  this.clearChatSubject.next(true);
}

is this the best approach? Is there a better way? I find it a bit wired to have a clearChatSubject that can be true or false.
also the part where I merge the messages from rest and WebSockets feels wired to me.
Thank you

[solid crystal]

Not sure what your intentions are, but:

• Your app needs to wait that the initialMessages are loaded before being able to show the newMessages
• If you use clearChat before the initialMessages are resolved, the result will be a []

[zealous nacelle]

yes this not a big problem in my usecase. Would be nice if this edgecase would also work but it makes the code even more complicated I guess

[woven bobcat]

@Component({
  template: `
<div *ngFor="let message of messages$ | async>{{ message }}</div>
<button (click)="clearChat()">Clear Chat</button>
`,
})
export class MessageListComponent {
  public readonly messages$: Observable<string[]>;

  constructor(private readonly service: MessagesService) {
    this.messages$ = this.service.messages$;
  }

  public clearChat(): void {
    this.service.clearChat();
  }
}

Most of what you have in that Component should probably be in the Service IMO.

[zealous nacelle]

thank you

[woven bobcat]

And I think you have too many Subjects in there. But without seeing the shatSocketService I dunno what exactly is going on.
But IMPO, code that does things belongs in a Service
Components are to create DOM and attach Services to it

[zealous nacelle]

there was a typo, it should be chatSocketService. This gets messages from socketIo. Every now and then an array of messages will be sent over WebSockets. The real code is a litte more complicated.
I agree, business logic belongs in a service. But i am more interested in my Observable messages$. Is it fine or is the a way to make it better?

thinks that matter:

• initial messages loaded through REST
• new messages through Socket.io, it's an observable of an array of messages
• ability to clear the chat (client side)

[woven bobcat]

Quick note. .pipe() already turns a Subject into an Observable. you don't need .asObservable()

Well without types and comments I have no idea what it is doing. Your logic for clearing the messages seems convoluted to me. Not sure why that wouldn't just be

this.messages$ = merge(
  this.chatRestService.getMessages$(this._chatRoomId), // Should also already handle getMessageEvents$ IMPO
  this.clearChatSubject.pipe(map((): string[] => [])),
);

[solid crystal]

If that can help, I quickly created a POC during my lunch: https://stackblitz.com/edit/angular-ivy-pdsyic?file=src/app/app.component.ts
I don't really know how to reset the chat though. But I'm interested by the solution (could help me someday 😁)

[zealous nacelle]

I was using combineLatest but then I cannot clear the chat. this is why I was using concatMap and switchMap

[woven bobcat]

yeah, that is why I used merge

[zealous nacelle]

I added the type to initialMessages$ and newMessages$ exactly for your understanding.
anyway I love your solution using merge 💯 this is exaclty what I was looking for! thank you very much!!!!

actually clearChatSubject can emit an array istread of boolean, this is even better 😄

[woven bobcat]

Assuming it works.
I'm just not seeing a case where you would ever want initialMessages$ that didn't also include newMessages$

[zealous nacelle]

well in a chat you have some old messages stored in the DB, I get those through REST.
Then once the chat is loaded, new messages will arrive through WebSockets. I need to combine those streams....

it works so far...

[woven bobcat]

yes, but you should combine those steams in your Service. The job of your Component isn't to know how the backend is structured. You need to hide that way from the poor little Component that just wants to make some DOM to show the user.

[zealous nacelle]

I agree whit that, but in order to show the code here, I packed everything relevant in the same component, so it's one single file.

[solid crystal]

Ok but how about the newMessages recieved (with the socket) after the initial loading (with rest)?

We really need the service detail here. That's the point of this issue I guess.

But I agree that it should be in a service

[woven bobcat]

@Injectable({ providedIn: 'root' }) // Alwas 'root' unless you _know_ better!
export class ChatRestService {
  constructor(private readonly http: HttpClient) {}

  public getMessages$(id: string): Observable<string[]> {
    return combineLatest([
      this.http.get<string[]>(`/api/chat-room/${id}/messages`),
      this.getMessageEvents$(id),
    ]).pipe(
      map(([ initialMessages, newMessages ]: [ string[], string[] ]): string[] => [ ...initalMessages, ...newMessages ]),
    );
  }
}

[solid crystal]

That's why I created a POC because I don't really know how to achieve this scenario and I believe it's worth the learning.

[solid crystal]

I'm sorry but it wont work

[zealous nacelle]

how do you clear the chat?

ok I am gonna create a POC

[solid crystal]

this.messages$ = merge(
  this.chatRestService.getMessages$(this._chatRoomId), // Should also already handle getMessageEvents$ IMPO
  this.clearChatSubject.pipe(map((): string[] => [])),
);

The messages will be reset to previous values in the following scenario:

• clear -> Will display []
• a new message is coming from the socket -> will dispay the old messages + the new one

Or am I missing something?

[solid crystal]

Already created one...

[woven bobcat]

You would want to move the "clearChat" method into the Service and have it handle that.
There is a lot to type here and since I haven't seen the implementation code I cannot just copy and paste it.

[solid crystal]

That's why I created a POC https://stackblitz.com/edit/angular-ivy-pdsyic?file=src%2Fapp%2Fapp.component.ts

Sorry but I'm just trying to understand because the subject interests me. Thanks for your help btw

[solid crystal]

That's exactly the implementation of "have it handle that" that I would like to learn from 😅

[zealous nacelle]

one that works with clearChat: https://stackblitz.com/edit/angular-ivy-eong2d?file=src/app/app.component.ts

I know that stuff should be in services, but right now I am only interested in the observable, and the location of the code doesn't matter in this scenario

[woven bobcat]

Disagree... but you do you 🙂

[solid crystal]

You code is not bullet proof, change your delay to 5000 and you'll see that it will clear your initial messages

But it's better than nothing, I leave you that

I thought there was a more elegant method 😔

[zealous nacelle]

I know... I can live with that... I am sure there is a solution but I don't know it yet

[solid crystal]

Yeah, I understand. That was a good question. I just hoped that I would end the day being smarter than this morning 😅

[zealous nacelle]

I learned that in this case merge comes handy. It's a good improvement compared to the initial code

I have an idea... let me test

[zealous nacelle]

@solid crystal i think I got it 😀
have a look if you are interested: https://stackblitz.com/edit/angular-ivy-zvidac?file=src/app/app.component.ts

looks really clean, I hope there is no error