This differential refactors useJoinThread hook so that it handles DM threads as well.
Details
Details
- Reviewers
kamil tomek inka ashoat - Commits
- rCOMMa91bc4cfd0f7: Implement joining DM thread
TODO. Once Inka implements creating thick sidebars testing will be easy and smooth.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-9171
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/actions/thread-actions.js | ||
|---|---|---|
| 354–356 ↗ | (On Diff #43941) | This API is confusing because it's unclear when rawThreadInfo needs to be specified. One possibility would be to split into two hooks: useJoinThinThread and useJoinThickThread. However, all the callsites would have to call each hook separately, and the ChatInputBars would need some additional code to decide which to use. Another option would be to replace the input here. Combined with the above feedback, I think we could do an API like this: type UseJoinThreadInput = | $ReadOnly<{ +thick: false, ...ClientThreadJoinRequest, }> | { +thick: true, +rawThreadInfo: RawThreadInfo, }; function useJoinThread(): ( input: ClientThreadJoinRequest, ) => Promise<ThreadJoinPayload> { ... } |
| 355 ↗ | (On Diff #43941) | There's no reason for this to be a parameter to the hook instead of part of input. It's more flexible to move it to input, so let's do that |
| native/chat/chat-input-bar.react.js | ||
| 1139–1142 ↗ | (On Diff #43941) | Do we want to use joinThreadActionTypes for thick threads? Does processAndSendDMOperation dispatch a Redux action, in which case we're dispatching an action from another action? That's generally an anti-pattern, and makes me wonder if we really need to be dispatching a Redux action at all. If we take the approach of splitting into useJoinThinThread and useJoinThickThread, then we could document this difference in behavior (useJoinThickThread dispatches an action and useJoinThinThread doesn't), and make sure that ChatInputBar only wraps useJoinThinThread in dispatchActionPromise. |
| lib/actions/thread-actions.js | ||
|---|---|---|
| 354–356 ↗ | (On Diff #43941) | I think this solution has the same disadvantage as separate hooks since the component has to know whether it is running for thick thread to device on the input structure. Is there anything that I am missing? Perhaps you are aware of this but don't consider it a problem since we can use simple ternary statement to choose the right input? |
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 1139–1142 ↗ | (On Diff #43941) | I think the reason we want to use dispatchActionPromise in thick threads is to be able to call createLodaingStatusSelector. processAndSendDMOperation alone doesn't let us track the status of an action. cc @tomek can you confirm? |
| lib/actions/thread-actions.js | ||
|---|---|---|
| 354–356 ↗ | (On Diff #43941) | Yeah, that makes sense. I think we should probably go with separate hooks... it seems beneficial also for the reasons described in the inline comment I left on lines 1139-1142 |
| lib/actions/thread-actions.js | ||
|---|---|---|
| 354–356 ↗ | (On Diff #43941) | Have you seen my comment there: https://phab.comm.dev/D13260#inline-76487? I think it could affect your judgement. |
Comment Actions
I still think the API is pretty confusing, but it's probably too much work here to address the Redux-action-wrapping-another-Redux-action issue.
Could you:
- Create a follow-up task for that?
- Improve the API through either of the suggestions from my comment on lines 354-356?
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 1139–1142 ↗ | (On Diff #43941) | Ah, that's helpful context. I can see now why dispatching a Redux action from another action is the most expedient solution here. The alternative would be to track multiple loading statuses from each component. If processAndSendDMOperation dispatches via dispatchActionPromise, this wouldn't be too hard... we could do this with a combination of customKeyName and combineLoadingStatuses. But I suspect processAndSendDMOperation doesn't dispatche via dispatchActionPromise. |
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 1139–1142 ↗ | (On Diff #43941) | The useProcessAndSendDMOperation hook indeed does use dispatchActionPromise: https://github.com/CommE2E/comm/blob/d1ef059b2362927a7f646a7616b1c70e4e31d0d1/lib/shared/dm-ops/process-dm-ops.js#L335
|
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 1139–1142 ↗ | (On Diff #43941) |
That's great! Let me know if you have any questions about how to use customKeyName and combineLoadingStatuses here. I think you'll need to change the useProcessAndSendDMOperation API so that the function returned there takes the customKeyName as input. I can share some more pointers if helpful.
Defer to you, but please prioritize readability!
I think the API should make it clear when rawThreadInfo needs to be specified. I suggest two approaches to this: either implementing separate useJoinThinThread and useJoinThickThread hooks, or by having a single useJoinThread with an input similar to the UseJoinThreadInput type I proposed above. I'm open to either solution, or an alternative that make it clear when rawThreadInfo needs to be specified. |
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 1139–1142 ↗ | (On Diff #43941) |
If we proceed with one hook with union of inputs, then why can't we use dispatchActionPromise as it is used in this diff? |
| native/chat/chat-input-bar.react.js | ||
|---|---|---|
| 1139–1142 ↗ | (On Diff #43941) | I'm not sure I understand the question. Are you confused about why it's bad practice to dispatch an action from another action? |
| lib/actions/thread-actions.js | ||
|---|---|---|
| 354–356 ↗ | (On Diff #43941) | I described some different possible approaches here - we can discuss what is better there. |
| native/chat/chat-input-bar.react.js | ||
| 1139–1142 ↗ | (On Diff #43941) |
The code you linked is only for non auto retriable messages, here, to use dispatchActionPromise you'll some additional changes to support that. |
| lib/actions/thread-actions.js | ||
|---|---|---|
| 355 ↗ | (On Diff #44015) | You implemented most of my suggestion, but skipped the $ReadOnly wrapper. Can you make sure to add that before landing? |