Can you provide some more context about this? I don't quite understand why it makes sense to start from an action... my initial thinking was that business logic would construct a DMOperation directly, rather than constructing an action that gets converted into a DMOperation

In D12776#361491, @ashoat wrote:

Can you provide some more context about this? I don't quite understand why it makes sense to start from an action... my initial thinking was that business logic would construct a DMOperation directly, rather than constructing an action that gets converted into a DMOperation

What do you mean by business logic? Is it the logic that lives in components, or in reducers, or maybe somewhere else?

I can see at least three possible places where DMOperations can be generated

1. In reducers
2. In specs called from a reducer - this implementation
3. In components

I don't have a strong opinion about which one is better for us. Centralizing this logic sounds beneficial - they can be defined in some kind of specs, but we can call the specs from many places.

We also have some constraints in our solution that limit how we can handle the DMOperation

1. Each OutboundP2PMessage needs to be put into the DB.
2. All the items that go to our DB, need to be queued using queueDBOps.
3. Then DBOpsHandler takes from the queue and saves items into the DB.

This means that without significant changes each OutboundP2PMessage needs to go through Redux. So if we decide to generate the DMOperations in components, we will need to introduce a new action that passes through Redux. This action will also need to modify the store and perform the same state modification that will happen on recipients. That's why starting with an action makes some sense - we already have the reducer logic that modifies the store based on it, and the only thing that is missing is creating the operation from it - similar to how we create DB operations.

What do you mean by business logic? Is it the logic that lives in components, or in reducers, or maybe somewhere else?

I was thinking of the logic in components.

I can see at least three possible places where DMOperations can be generated

1. In reducers
2. In specs called from a reducer - this implementation
3. In components

I don't have a strong opinion about which one is better for us. Centralizing this logic sounds beneficial - they can be defined in some kind of specs, but we can call the specs from many places.

We also have some constraints in our solution that limit how we can handle the DMOperation

1. Each OutboundP2PMessage needs to be put into the DB.
2. All the items that go to our DB, need to be queued using queueDBOps.
3. Then DBOpsHandler takes from the queue and saves items into the DB.

This means that without significant changes each OutboundP2PMessage needs to go through Redux. So if we decide to generate the DMOperations in components, we will need to introduce a new action that passes through Redux. This action will also need to modify the store and perform the same state modification that will happen on recipients. That's why starting with an action makes some sense - we already have the reducer logic that modifies the store based on it, and the only thing that is missing is creating the operation from it - similar to how we create DB operations.

Hmm... to me, I think it would be much more simple to have the components generate a DMOp, and then to have a utility function that calls processDMOperation on it, and put the result in a Redux action. That way, we avoid having to support multiple different kinds of actions. What do you think?

Hmm... to me, I think it would be much more simple to have the components generate a DMOp, and then to have a utility function that calls processDMOperation on it, and put the result in a Redux action. That way, we avoid having to support multiple different kinds of actions. What do you think?

Yeah, agree, it's simpler. It's similar to an updated solution I was thinking about. But there are some things we have to figure out:

1. As of D12780 processDMOperation returns rawMessageInfos and updateInfos. But we also need to generate OutboundP2PMessages (generateMessagesToPeers function from this diff) - we probably should do that as a part of processDMOperation.
2. We need to know the sending status of messages that aren't automatically retriable. We can have a function that takes OutboundP2PMessages and returns a promise that can be passed to dispatchActionPromise.
3. Manual retries need IDs of failed messages - that is something relevant only for local messages. But we will need to modify them in a way that allows storing the IDs.

Figured sharing these makes us sure we're on the same page.

Would probably be a good idea to discuss this in my detail in our 1:1 tomorrow. One tradeoff neither of us has mentioned so far about using a single action for all business logic that generates DMOperations is that it might be debugging a little bit more painful.

But we also need to generate OutboundP2PMessages (generateMessagesToPeers function from this diff) - we probably should do that as a part of processDMOperation.

Hmmm... but doesn't processDMOperation get called on the peer to process the DMOperation sent by somebody else? It seems like we should only call generateMessagesToPeers on the sender's side.

2. We need to know the sending status of messages that aren't automatically retriable. We can have a function that takes OutboundP2PMessages and returns a promise that can be passed to dispatchActionPromise.

A little confused about this one. Can you explain when / where we need to know this "sending status"? Will the business logic in components need to call this function and then pass it to dispatchActionPromise?

3. Manual retries need IDs of failed messages - that is something relevant only for local messages. But we will need to modify them in a way that allows storing the IDs.

I'm confused about this one too. What needs to be modified? What sort of IDs need to be stored? I thought this data was available in the MessageStore already, but I might be missing something.

In D12776#361904, @ashoat wrote:

Would probably be a good idea to discuss this in my detail in our 1:1 tomorrow. One tradeoff neither of us has mentioned so far about using a single action for all business logic that generates DMOperations is that it might be debugging a little bit more painful.

Yes, definitelly.

But we also need to generate OutboundP2PMessages (generateMessagesToPeers function from this diff) - we probably should do that as a part of processDMOperation.

Hmmm... but doesn't processDMOperation get called on the peer to process the DMOperation sent by somebody else? It seems like we should only call generateMessagesToPeers on the sender's side.

Yes, we should generate messages only on sender's side. I was considering introducing a flag or something, but ultimately created a different solution D12786 where I create a new method sendDMOperation in PeerToPeerProvider.

2. We need to know the sending status of messages that aren't automatically retriable. We can have a function that takes OutboundP2PMessages and returns a promise that can be passed to dispatchActionPromise.

A little confused about this one. Can you explain when / where we need to know this "sending status"? Will the business logic in components need to call this function and then pass it to dispatchActionPromise?

I'm not sure. E.g. in the input state container, we are checking the sending status of text messages. If our logic really needs that, we can call sendDMOperation and pass the result to dispatchActionPromise.

3. Manual retries need IDs of failed messages - that is something relevant only for local messages. But we will need to modify them in a way that allows storing the IDs.

I'm confused about this one too. What needs to be modified? What sort of IDs need to be stored? I thought this data was available in the MessageStore already, but I might be missing something.

The missing data is the IDs of messages that are sent through tunnelbroker (OutboundP2PMessage). We're storing the messages in SQLite, but in order to retry them, we need their IDs. (there are some alternative approaches, e.g. sending messages to all the peers even if a message failed only for some - easier, but wasteful, and could be incorrect).

Thanks for all of the explanations above, those all make sense. @tomek and I discussed this in our 1:1 today and settled on keeping this approach, at least for now.