Depending on the use case, it might be worth considering to further process this into something more efficient. On the client side we would probably need a count for each reaction and some indication if a viewer also reacted. But we can also do this optimization later. (one case where it might be really beneficial is when someone liked and unliked a message huge number of times. In the current approach, the resulting array will be long, but in the proposed one it will just be a number 0 or 1).

It is a really minor thing, but you can avoid having to specify the inserted item twice

Always reacting to the source message makes sense to me!

Just put up D5853 and this should address these concerns!

I took a stab at this, but I feel like is out of the scope of this diff. This is still a great point though, and I will create a follow up task to address this

It might be cleaner to use an IIFE for this, eg.

const renderedReactions: $ReadOnlyMap<string, MessageReactionInfo> = (() => {
  let result;
  ...
  return result;
})();

That way we group all the code that sets up renderedReactions in one place, and the reader knows we aren't changing it later

Right now, we are only showing how many users like a message, so we could just add the id of the user; however, I was trying to think a couple of steps ahead, and I know that we eventually will want to show some information about the user like the name for example. So it made sense to me to store the whole object into userSet so that when we are ready to work on showing that type of information, we have it readily available.

We also want to store the object as a string, because, from what I understand, it is not possible to remove the user object from the set, because they won't have the same reference, and converting the object into the string will bypass this issue. More context here.

From what I understand, if we are just storing the user ids in the usersSet then using viewerId when rendering would be more simple, but since we have more data about the user than just the user id, I believe what we have right now would be more simple. However, if you think I am missing something please let me know!

I like that idea a lot, definitely makes sense and avoids the serialization/deserialization concern. For now, since we are only showing the number of likes for the first version of message likes, I am just going to store the user id into the set, but will create a follow-up task for creating the (userID -> creator) map for when we are ready to show more user info on who reacted to a message

Decided to go with your initial suggestion of using viewerID. Thanks for the suggestion, definitely thought that it made the overall solution more elegant!

We need to iterate backwards here because if we do not, the order is from most recent to oldest, and we could run into the case where the most recent reaction message has the remove_reaction as its action. The code would then try to remove a user that hasn't been put into the usersSet already, essentially just skipping that unlike

Could be good to add a code comment explaining that

We need to iterate backwards here because if we do not, the order is from most recent to oldest, and we could run into the case where the most recent reaction message has the remove_reaction as its action. The code would then try to remove a user that hasn't been put into the usersSet already, essentially just skipping that unlike

ohhhh, yeah totally makes sense

Follow up task

O6 is not for code comments, it's for externally-facing documentation

Just figured they're generally the experts on the English language and would be best suited to review code comments as well.