[web] Add identity client shared worker proxy
ClosedPublic
Actions

Authored by michal on Mar 7 2024, 8:27 AM.

Details

Reviewers

kamil
tomek

Commits

rCOMM14ef5bb851df: [web] Add identity client shared worker proxy

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

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Mar 7 2024, 8:27 AM

Herald added a subscriber: ashoat. · View Herald TranscriptMar 7 2024, 8:27 AM

Harbormaster completed remote builds in B27390: Diff 37933.Mar 7 2024, 8:43 AM

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.

Harbormaster completed remote builds in B27404: Diff 37950.Mar 8 2024, 5:12 AM

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.

michal added a child revision: D11299: [web] Handle shared worker message sequentially.Mar 11 2024, 6:35 AM

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.

In D11275#325678, @ashoat wrote:

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?

Rebase

Harbormaster completed remote builds in B27501: Diff 38058.Mar 14 2024, 4:46 AM

michal mentioned this in D11338: [web] olmAPI on shared worker.Mar 15 2024, 10:19 AM

kamil accepted this revision.Mar 18 2024, 3:54 AM

This revision is now accepted and ready to land.Mar 18 2024, 3:54 AM

Closed by commit rCOMM14ef5bb851df: [web] Add identity client shared worker proxy (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyMar 18 2024, 6:42 AM

This revision was automatically updated to reflect the committed changes.

Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMM14ef5bb851df: [web] Add identity client shared worker proxy.

ashoat added inline comments.Mar 18 2024, 6:37 PM

web/shared-worker/worker/identity-client.js
40 ↗	(On Diff #38137)	I prefer `any`-casts to `$FlowFixMe` in cases where there is nothing to fix 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);