Refactor the local field in the message reducer + relevant util functions in this file to use the ops approach
Depends on D12357
Details
Details
Tested each action type w/ @will during the hackathon, and we were able to confirm that we were getting the expected behavior for each action
Some of the main user flows we tested:
- fail sending a message
- successfully retry sending a message
- closing the app where a thread had a bunch of sent failed local messages
- fail sending a reaction message
- deleting a thread with a bunch of sent failed messages
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 791–792 | Why don't we set the local here? Shouldn't we set it for all the actions? Maybe we can even spread the processed store? | |
| 901 | Shouldn't we also set the local? Could you explain here, and in all the other places, why we're not setting the local? | |
| 1583–1586 | It is surprising to see an action where we're adding some ops without removing the old logic. Originally, we weren't modifying the messageStore.local, and now we do. What is the reason? | |
Comment Actions
We should also check for store inconsistencies in setClientDBStoreActionType - but I guess that extending this action will be done later in the stack.
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 791–792 | Was thinking that since messageStore.local was not being touched it would be fine to not explicitly set local here, but now I can see that it would be best to be explicit here and also set local | |
| 1583–1586 | I think initially during the hackathon the thinking was that since these are the ops for the createLocalMessageActionType it might make sense to add a "shell" local message info; however, after thinking about it some more, this is unnecessary/redundant, and can be removed | |
Comment Actions
We should also check for store inconsistencies in setClientDBStoreActionType - but I guess that extending this action will be done later in the stack.
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 1583 ↗ | (On Diff #41228) | Noticed we were missing setting threads here so snuck this change in |
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 791–792 | For me, the benefit of having local here is that the logic from the reducer and from the ops processing get decoupled - the reducer doesn't need to know any detail about how the result of ops processing will look. | |