issue: https://linear.app/comm/issue/ENG-4728/refactor-useservercall-step-3-implement-fanout-logic
Fanout actions are actions that call all keyservers.
Details
Details
Created an object for searchMessages:
const searchMessagesActionObj: KeyserverCall<
[SearchMessagesRequest],
SearchMessagesResponse,
> = {
actionFunc: searchMessages,
config: { keyserverSelection: 'fanout' },
};Called this action from the message search modal, checked that the results appear, and that it is possible to load more results
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Overall looks good but I have a question. In the test plan you write:
const searchMessagesActionObj: KeyserverCall<
[SearchMessagesRequest],
SearchMessagesResponse,
> = {
actionFunc: searchMessages,
config: { keyserverSelection: 'fanout' },
};but shouldn't the return type be [SearchMessagesResponse] as it's a fanout action?
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 107 ↗ | (On Diff #31240) | I meant that this Return is wrong, because in case of fanout it returns [Return] |
Comment Actions
I just realized a problem with approach: we don't want to send all of the input to all of the keyservers. For example in D9240, in fetchSingleMostRecentMessagesFromThreads we want to send each keyserver only the ids that it "owns". Otherwise we could expose private date to other keyservers. We need some logic to split a single request into multiple request, one for each keyserver.
Comment Actions
Chenge implementation to allow dividing data per keyserver. Reasoning explained here: https://linear.app/comm/issue/ENG-4440/refactor-redux-fields-second-iteration-refactor-actions#comment-f5d6c650
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 97 ↗ | (On Diff #31606) | Should we replace Object with { +[string]: mixed }? |
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 97 ↗ | (On Diff #31606) | In some cases, when an action used to take for example a string, I left the requests in the form of {[string]: string} to lessen the amount of boilerplate, so { +[keyserverID: string]: { +[string]: mixed } } won't work |
Comment Actions
Requesting changes to make sure I at least get a satisfactory explanation before this lands (ideally a proper solution)
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 97 ↗ | (On Diff #31606) | Can you please clarify further why it "won't work"? Is the issue with { +[string]: mixed } that it's read-only? If so, you should see if you can convert the {[string]: string} cases to read-only. If that doesn't work, you can consider { [string]: mixed }. I know it was Object before, but it's our responsibility as stewards of the codebase to always improve types when we touch them. Object is particularly bad, as it's basically synonymous with any. We should strive to remove all anys from the codebase. |
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 97 ↗ | (On Diff #31606) | I meant that if before the request was a string, then I left it as string, so the requests is in that case a { +[keyserverID]: string }, so not { +[keyserverID: string]: { +[string]: mixed } } I can change this by either:
The first option adds a lot of boilerplate, that's why I wanted to avoid it. The second option should probably have been used in the first place, since that's how CallKeyserverEndpoint is typed currently, and we have a task to update it at some point: ENG-4968 |
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 97 ↗ | (On Diff #31606) | Thanks for explaining! Two follow-ups here:
|
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 97 ↗ | (On Diff #31606) | I went through all actions and actually it looks like I have been creating the auxiliary types. I got it mixed up with that I didn't want to create them for action inputs, not for requests. I'm very sorry about this, I will update the type |
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 129–133 | Unclear why this promise stuff was necessary here... bindCallKeyserverEndpoint(keyserverID) does not return a Promise | |
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 129–133 | Hmm actually, looking at this again I think I'm just confused by some naming | |
| lib/utils/keyserver-call.js | ||
|---|---|---|
| 129–133 | The only issue was actually mine in D9748... I typed promises as { [string]: Promise<CallServerEndpoint> }, when it is actually { [string]: Promise<Object> }. I got confused by the naming here... bindCallKeyserverEndpoint seemed to imply that the function bound the CallKeyserverEndpoint function, but really it's making the server call to one specific keyserver. Flow didn't catch it because Object is basically any | |