introduced sendReactionMessageActionTypes. These will be used in subsequent diffs mainly in message-reducer
Linear Task: ENG-2248
Details
Details
flow also ran app locally on web/native and nothing crashed and there were no regressions. Will continue testing in subsequent diffs
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
working on message reducer stuff right now and I have a strong intuition that SendReactionMessagePayload will need to change
| lib/types/message-types.js | ||
|---|---|---|
| 521–536 ↗ | (On Diff #19048) | Not sure about the plan here, but it seems inconsistent:
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. |
| 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? |
| 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. |
| 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 |
| 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 |
Comment Actions
Some questions about SendReactionMessagePayload
| lib/types/message-types.js | ||
|---|---|---|
| 547 ↗ | (On Diff #19332) | Can you explain why we have this newMessageInfos field?
|
Comment Actions
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 ↗ | (On Diff #19332) | 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. |
Comment Actions
@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
Comment Actions
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.