Page MenuHomePhabricator

[lib] Update `messageStore.messages` with `msgStoreOps` for logIn.success action
ClosedPublic

Authored by atul on Jun 2 2022, 5:06 PM.
Tags
None
Referenced Files
F3298742: D4201.diff
Sun, Nov 17, 8:21 AM
Unknown Object (File)
Sun, Nov 10, 6:41 AM
Unknown Object (File)
Sat, Nov 9, 11:41 AM
Unknown Object (File)
Sat, Nov 9, 11:41 AM
Unknown Object (File)
Sat, Nov 9, 11:41 AM
Unknown Object (File)
Fri, Nov 8, 8:08 AM
Unknown Object (File)
Tue, Nov 5, 2:02 AM
Unknown Object (File)
Thu, Oct 24, 7:56 AM

Details

Summary

Instead of getting messageStore.messages from the freshStore returned by freshMessageStore... we use the messageStoreOps returned by freshMessageStore to compute the new message store.

Note that a messageStore.messages is still being created within freshMessageStore(...) and will be removed in a subsequent diff

Test Plan

This should be a straightforward refactor given we've had assertMessageStoreMessagesAreEqual in place for some time now.

Diff Detail

Repository
rCOMM Comm
Branch
landjune21 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul requested review of this revision.Jun 2 2022, 5:13 PM
tomek requested changes to this revision.Jun 3 2022, 4:26 AM
tomek added inline comments.
lib/reducers/message-reducer.js
736 ↗(On Diff #13316)

Are you sure using freshStore is a good idea here? Maybe processedMessageStore should be used instead?

This revision now requires changes to proceed.Jun 3 2022, 4:26 AM
atul requested review of this revision.Jun 17 2022, 1:32 PM
atul added inline comments.
lib/reducers/message-reducer.js
736 ↗(On Diff #13316)

Yeah, because right now the "ops" only modify messageStore.messages... we need to modify the other properties of messageStore (eg threads, local, etc) "the old way"

This revision is now accepted and ready to land.Jun 20 2022, 2:33 AM

I still think that we should modify this whole stack so that a result of processMessageStoreOperations could be used as a new value of messageStore. We can e.g. modify the old messageStore and pass it as an argument to processMessageStoreOperations function.

rebase after cherrypicking and before addressing feedback

In D4201#121171, @palys-swm wrote:

I still think that we should modify this whole stack so that a result of processMessageStoreOperations could be used as a new value of messageStore. We can e.g. modify the old messageStore and pass it as an argument to processMessageStoreOperations function.

That's the goal, but getting it to that state will happen later in this stack.

If I understand correctly, I think you're suggesting that instead of ignoring the messageStore.messages from freshMessageStore(...) and substituting in the processedMessageStore.messages from the call to processMessageStoreOperations(...)... we should instead pass the output of freshMessageStore(...) into processMessageStoreOperations(...) and return the resulting messageStore.

I think that makes sense, but I don't think we can do it yet because we'd end up "applying the changes" twice. We'll need to make some modifications to freshMessageStore(...) and the callsites before we can get to that state. The goal of these first diffs is mainly to get assertMessageStoreMessagesAreEqual(...) "out of the way" to make room for future refactoring passes.