Page MenuHomePhabricator

[lib] Create useKeyserverCall
ClosedPublic

Authored by inka on Sep 11 2023, 3:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 11:45 AM
Unknown Object (File)
Sun, Nov 10, 9:38 PM
Unknown Object (File)
Sun, Nov 10, 5:06 PM
Unknown Object (File)
Wed, Nov 6, 1:49 AM
Unknown Object (File)
Sat, Nov 2, 6:32 AM
Unknown Object (File)
Sun, Oct 27, 10:19 PM
Unknown Object (File)
Sun, Oct 27, 10:19 PM
Unknown Object (File)
Sun, Oct 27, 10:19 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-4680/refactor-useservercall
We need to refactor useServerCall function (lib/utils/action-utils.js). Before there was only one keyserver, but as we move into multikeyserver world, an action might want to call one specific keyserver, or all of them. For each action we will thus have to define an object of type

export type KeyserverCall<Args: $ReadOnlyArray<mixed>, Return> = {
  +actionFunc: ActionFunc<Args, Return>,
  +config:  
   | { +keyserverSelection: 'fanout' }
   | {
      +keyserverSelection: 'specific',
      +keyserverIDExtractor: (...Args) => string,
    },
};

Where "fanout" means that we call all keyservers, and keyserverIDExtractor is a function that can extract keyserver id from actions arguments.

We cannot completely remove useServerCall function yet, because we would first need to create such objects for all actions. So for now I'm creating useKeyserverCall alongside useServerCall (the name is supposed to be changed
anyway).

In useServerCall implementation memoization was done using a combination of _memoize, createSelector and useMemo. I will do this in the next diff, and provide an explanation of what it does exactly. Thats why
createBoundServerCallsSelector is omitted for now.

See test plan for example of usage

Test Plan

Refactored serachMessages, and created a searchMessagesActionObj:

const searchMessages =
  (
    callServerEndpoint: CallKeyserverEndpoint<[SearchMessagesRequest]>,
  ): ((request: SearchMessagesRequest) => Promise<SearchMessagesResponse>) =>
  async request => {
    const response = await callServerEndpoint('search_messages', request, [
      request,
    ]);

    return {
      messages: response.messages,
      endReached: response.endReached,
    };
  };

const searchMessagesActionObj: KeyserverCall<
  [SearchMessagesRequest],
  SearchMessagesResponse,
> = {
  actionFunc: searchMessages,
  config: { keyserverSelection: 'specific', keyserverIDExtractor: () => '256' },
};

in useSearchMessages fined callSearchMessages as:

const callSearchMessages = useKeyserverCall(searchMessagesActionObj);

and checked that searching messages works correctly.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka edited the summary of this revision. (Show Details)
inka edited the summary of this revision. (Show Details)
lib/utils/keyserver-call.js
17 ↗(On Diff #30913)

This means that we have to pass an array to callKeyserverEndpoint. I don't know if there is a better way.
Ex in legacySendMultimediaMessage it would be:

const response = await callServerEndpoint('create_multimedia_message', payload, [threadID, localID, mediaIDs, sidebarCreation], { getResultInfo });
62–65 ↗(On Diff #30913)

fanout logic will be implemented in later diffs

inka requested review of this revision.Sep 11 2023, 4:13 AM
lib/utils/keyserver-call.js
14–19 ↗(On Diff #30914)

Can we type this better, avoiding any (and avoiding *, Object, etc.)?

60 ↗(On Diff #30914)

What is this TODO about? Please add more detail

62 ↗(On Diff #30914)

What is the point of Promise.all([])? Are you looking for Promise.resolve(undefined)?

Address review

lib/utils/keyserver-call.js
14–19 ↗(On Diff #30914)

This is any object returned from fetch or socketAPIHandler. Both in callServerEndpoint and in APIResponseServerSocketMessage we have this typed as Object. Can I create a followup task to create a type for all possible objects returned from callServerEndpoint?

60 ↗(On Diff #30914)

This is to be implemented in next diffs in this stack.

lib/utils/keyserver-call.js
16 ↗(On Diff #31163)

Is there no better way to do this? Sorry if I missed some prior discussion, feel free to link it. In general any usage should be explained

lib/utils/keyserver-call.js
16 ↗(On Diff #31163)

In CallServerEndpoint this is typed as Object. I should change both of these anys to Object, as it was before (my IDE shows any where we use Object, thats why I used any). I know this is not much better, but considering that this is how it was before, would it be ok if I just create a followup task to create those types? It requires going through all of our actions and aggregating their types into one big type.

Unfortunate that we can't use ReturnType<T> and Parameters<T> from new versions of flow, might simplify it a little

lib/utils/keyserver-call.js
16 ↗(On Diff #31163)

Maybe this would work? I think it's only used as an arg to fetch

19 ↗(On Diff #31163)

We could probably type this any better if we added input validation with validators. We will need to do this anyway in the future because we can't control other keyservers and they could send incorrect data.

Probably more of a follow-up task though.

This revision is now accepted and ready to land.Sep 18 2023, 5:49 AM
lib/utils/keyserver-call.js
16 ↗(On Diff #31163)

Let's try @michal's suggestion of {[string]: mixed}, but if that doesn't work we can keep it as-is. It's my understanding that Object is a synonym for any in modern versions of Flow, so I'm not sure there are any benefits to keeping it as Object

would it be ok if I just create a followup task to create those types?

That would be great. Might be worth mentioning @michal's suggestions of ReturnType<T> and Parameters<T> as well, assuming they could potentially help get rid of the any.

19 ↗(On Diff #31163)

Would be great to have a follow-up task for the input validators, separate from the one about the any type

I created a followup task to update the types: ENG-4968

Don't forget to create one for the input validator idea too!

Made a task about validators ENG-4982 because I have more context on them.

This revision was automatically updated to reflect the committed changes.