Page MenuHomePhabricator

[web] Add identity client shared worker proxy
ClosedPublic

Authored by michal on Mar 7 2024, 8:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 3:08 AM
Unknown Object (File)
Fri, Oct 18, 7:20 AM
Unknown Object (File)
Fri, Oct 18, 4:34 AM
Unknown Object (File)
Tue, Oct 15, 7:25 PM
Unknown Object (File)
Tue, Oct 15, 7:25 PM
Unknown Object (File)
Tue, Oct 15, 7:25 PM
Unknown Object (File)
Tue, Oct 15, 7:25 PM
Unknown Object (File)
Tue, Oct 15, 7:25 PM
Subscribers

Details

Summary

[ENG-6767 : Introduce IdentityServiceClientWrapper proxy](https://linear.app/comm/issue/ENG-6767/introduce-identityserviceclientwrapper-proxy)

Adds IdentityServiceClientSharedProxy which proxies the identity service request to the shared worker. Each method gets a new worker request message and if it returns a value also a response message. IdentityServiceContextProvider can now optionally (behind a flag) return the proxy instead of the "real" identity client.

Depends on D11274

Test Plan
  • Make sure that if e.g. access token changes that the identity client on shared worker is recreated with new credentials
  • generateNonce/getKeyserverKeys/uploadOneTimeKeys/publishWebPrekeys/getInboundKeysForUser/getOutboundKeysForUser/deleteUser/logInPasswordUser (everything except for the logInWalletUser) and made sure that they work

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

michal requested review of this revision.Mar 7 2024, 8:43 AM

Seems like a lot of additional boilerplate we'll need every time we add a new IdentityServiceClient method. Is there a way to avoid that? I wonder if we could've had just a single new message type for all IdentityServiceClient invocations

@ashoat would something like this be more preferable?

Instead of defining new message types for each method we instead have:

// Tab -> Worker request
export type CallIdentityClientMethodRequestMessage = {
  +type: 14,
  +method: $Keys<IdentityServiceClient>,
  +args: $ReadOnlyArray<mixed>,
};
// Worker -> Tab response
export type CallIdentityClientMethodResponseMessage = {
  +type: 4,
  +result: mixed,
};

Now defining a new identity method will only take:

  • a logic implementation in the IdentityServiceClientWrapper (this needs to be done anyway)
  • One line in proxy: functionName: (arg: number) => Promise<ResultType> = this.proxyToWorker('functionName');

proxyToWorker returns a function that takes any arguments and returns any value (but this function is exposed to the outside world only by the typed method on the class). The returned function implements the tab<->worker communication and fills in the correct method name.

  • The passed in method name as a string is typed so that it has be to be one of the keys in IdentityServiceClient
  • There are any casts/ $FlowFixMe required on the boundaries when using values from the request/response messages (as they are typed as mixed)
    • I've tried to type the message more strictly (so e.g. the args type must match the method string according to the IdentityServiceClient interface) but I wasn't able to get it work. At some point flow gives up and just returns any.

Thanks for explaining that it would require weaker types and some any-casts. I'm curious what the other reviewers think about the two proposed approaches, and I'm also curious how expensive it might be to change the approach at this time.

I'm also curious how expensive it might be to change the approach at this time.

I've already updated the diff with the "one worker message for all identity api" approach if that's what you meant. So switching to one or the other won't cost us anything.

Thanks for explaining that it would require weaker types and some any-casts. I'm curious what the other reviewers think about the two proposed approaches, and I'm also curious how expensive it might be to change the approach at this time.

I really like the new approach with less boilerplate.
any casts are problematic but since we control both ends of the "pipe" (while changing something we modify both webapp and worker and test it together) I guess we can ignore the risk?

This revision is now accepted and ready to land.Mar 18 2024, 3:54 AM
web/shared-worker/worker/identity-client.js
40
  1. I prefer any-casts to $FlowFixMe in cases where there is nothing to fix
  2. It's always good to make sure that we immediately re-cast the any type, to avoid polluting the types
const method: Something = (identityClient[message.method]: any);