-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CHAT-1565] Sync own_reactions in events #606
Conversation
Size Change: -2.08 kB (0%) Total Size: 212 kB
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AnatolyRugalev So the change looks fine to me. I have a suggestion:
With current implementation we mutate the event object inside channelState class. And thats a little confusing. If we override the event object, I think it should be done in channel.ts
, where we handle the event. So something like following:
_handleChannelEvent(event) {
...
case 'reaction.new':
if (event.reaction) {
/**
* richReactionsData = {
* latest_reactions: {},
* own_reactions: {},
* reactions_count: {}
* }
*/
const richReactionsData = s.addReaction(event.reaction, event.message);
event.message = { ...event.message, ...richReactionsData};
}
}
This is assuming we are shipping it with #602
@mahboubii what do you think?
@vishalnarkhede I was trying to do this by returning entire message from |
@AnatolyRugalev heads up that #602 was just merged into master |
I am still working on this. More improvements are coming:
|
f7746a3
to
18dcdb1
Compare
After some experimenting I came to the conclusion that we don't need to sync Incoming events contain this fields, so we simply update the message with them in the state. |
6242a12
to
1787d2c
Compare
I will review this tomorrow :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
A bit harder to follow the call but dropped a lot of repetition, I think it's a great improvement in code quality.
@AnatolyRugalev might want to update PR title since only |
This PR fixes the issue of synchronizing
own_reactions
in incoming events.The original problem comes from the fact that
own_reactions
field is not populated for each individual client when broadcasting the event. But fortunately, we already sync this data in the channel state, so replacing related fields in the event object seems like a reasonable solution.