Details

lib/types/message-types.js
521–536 ↗	(On Diff #19048)	Not sure about the plan here, but it seems inconsistent: `SendReactionMessagePayload` contains `targetMessageID` but not `reaction`. It seems we should either include both or none. Also, why do we need `targetMessageID` when `newMessageInfos` is there? `SendMessagePayload` has `localID`. It seems like we can use a similar approach when sending reaction message - that id can be used to check what message was sent. `SendMessagePayload` doesn't need `newMessageInfos`. Why this is different comparing to `SendReactionMessagePayload`? It seems like using `SendMessagePayload` instead should be possible, but maybe I'm missing something.
lib/types/redux-types.js
536–537 ↗	(On Diff #19048)	It doesn't seem to scale well when we have different reaction types. It shouldn't be a big issue now, but we have to think about a case where a user has two devices: one which supports only liking and one with more reaction types.

This revision now requires changes to proceed.Dec 1 2022, 2:58 AM

addressed comments

Harbormaster completed remote builds in B14096: Diff 19296.Dec 12 2022, 2:09 PM

ginsu added inline comments.Dec 12 2022, 2:14 PM

lib/types/message-types.js
521–536 ↗	(On Diff #19048)	Sorry this took me a while to address Addressed the first point, your feedback makes sense, and having a separate `targetMessageID` field is unnecessary I believe that since reaction messages aren't being shown optimistically at the moment, we need to have a separate reaction message payload type since we are only showing reactions if it was successfully sent. When we eventually work on displaying reaction messages optimistically, I think then using `SendMessagePayload` should be possible.
lib/types/redux-types.js
536–537 ↗	(On Diff #19048)	I am honestly still a little confused about this feedback here; again, sorry this took me a while to address. Please correct me if I am wrong, but regarding the case that you provided about a user having two devices, `targetMessageID` and `threadID` will always be present on both devices because both liking and future reaction types will need a `targetMessageID` field and `threadID` field. Could you please elaborate more on what you mean regarding that this won't scale well?

tomek accepted this revision.Dec 13 2022, 3:28 AM

tomek added inline comments.

lib/types/message-types.js
521–536 ↗	(On Diff #19048)	Ok, that makes sense!
lib/types/redux-types.js
539 ↗	(On Diff #19296)	Why this is optional?
536–537 ↗	(On Diff #19048)	I think I was concerned about this because error payload doesn't contain an info about which reaction type failed. I don't think it is a valid concern because of how the rest of a stack looks like: in this code version we can assume `like` action and this code version would not receive a failed request about different reaction type - because it can't send it.

ginsu added inline comments.Dec 13 2022, 9:41 AM

lib/types/redux-types.js
536–537 ↗	(On Diff #19048)	Oh gotcha, thank you for elaborating this makes sense now, I will update the diff to have the reaction type in the error payload

address feedback

ginsu added inline comments.Dec 13 2022, 9:53 AM

lib/types/redux-types.js
539 ↗	(On Diff #19296)	I was initially basing this type from the other send message redux types (`SEND_TEXT_MESSAGE_STARTED`, `SEND_TEXT_MESSAGE_FAILED`, `SEND_TEXT_MESSAGE_SUCCESS`, `SEND_MULTIMEDIA_MESSAGE_STARTED`, etc), but after taking a second look and based on what I did on D5782, it seems like it would be better to make `loadingInfo` required

Harbormaster completed remote builds in B14132: Diff 19332.Dec 13 2022, 10:02 AM

Some questions about SendReactionMessagePayload

lib/types/message-types.js
547	Can you explain why we have this `newMessageInfos` field? It looks like `SendMessagePayload` doesn't include any equivalent field? It looks like anything regarding message contents are in `SendTextMessageRequest` or `SendMultimediaMessageRequest`, etc. Why do we need to be sending an array of `RawMessageInfo`s? Isn't it only possible to send one reaction at a time? Are you sure we need to be sending over the entire contents of `RawMessageInfo`? Can we narrow the type to `RawReactionMessageInfo`?

This revision now requires changes to proceed.Dec 15 2022, 9:05 AM

ginsu added inline comments.Dec 15 2022, 2:27 PM

lib/types/message-types.js
547	The reason we need to have `newMessageInfos` here, is because in D5782, we are using the `mergeNewMessages` function and it needs an array of `RawMessageInfos` as one of its parameters

ginsu attached a referenced file: F286718: Screenshot 2022-12-15 at 5.25.34 PM.png. (Show Details)Dec 15 2022, 2:27 PM

@ginsu you should Re-Request Review to move this back to @atul's queue if you think his question has been addressed

ginsu requested review of this revision.Dec 16 2022, 7:05 AM

Accepting but I think the creation of the RawMessageInfos array can be pushed down to message-reducer to keep things cleaner.

lib/types/message-types.js
547	Can we handle the creation of array of `RawMessageInfos` in `message-reducer` instead? I think that'd be cleaner than creating the array here just because a function called down the line needs it in that format.

This revision is now accepted and ready to land.Dec 18 2022, 8:08 PM

In D5769#176948, @atul wrote:

Accepting but I think the creation of the RawMessageInfos array can be pushed down to message-reducer to keep things cleaner.

@atul would you be okay if we punted this down to when I work on the displaying reactions optimistically task later in the week? I feel like this will go hand in hand with that task

In D5769#177981, @ginsu wrote:

In D5769#176948, @atul wrote:

Accepting but I think the creation of the RawMessageInfos array can be pushed down to message-reducer to keep things cleaner.

@atul would you be okay if we punted this down to when I work on the displaying reactions optimistically task later in the week? I feel like this will go hand in hand with that task

I mean we're defining the interface here with SendReactionMessagePayload so ideally we'd correct it right now, but I get that it'd be helpful to get as much of this stack landed as soon as possible so it's probably fine to punt it for now.