Is this the best way to handle getting the deviceID? One risk is that it's possible for it to not be set by the time commRustModule.getKeyserverKeys is called.

I wonder if we should instead do something like:

const deviceIDPromiseRef = React.useRef<?Promise<string>>();
if (!deviceIDPromiseRef.current) {
  deviceIDPromiseRef.current = getContentSigningKey();
}

And then wherever deviceID is needed, we just call await deviceIDPromiseRef.current instead.

Yeah, I think we should. It was also mentioned in https://linear.app/comm/issue/ENG-6404/move-identity-client-to-a-context#comment-52aa3177

If we go with the approach I proposed there, we can probably remove it from Redux, in favor of exposing it via this context

We'll also need an initial call to commCoreModule to get the current values of the auth metadata (the emitter just provides updates, I think)

How do we handle it if the content signing key changes?

I think that right now, this can happen when the user logs out (or is logged out) and then logs in.

In the future, this can happen due to a primary device key rotation (which should be invisible to the user), or a restore (which happens while logged out).

Nit: read-only

Why do we need the ? here?

Why do we need these runtime checks?

Is this an unexpected scenario? Should we do something more than just returning null here? Eg. a console.log, throwing an exception, etc.

Feels a little inconsistent that we use "Auth" vs "Unauthorized". There was a discussion in D10430 and we settled on "Unauth" for this... do you think you can rename?

Similar question about error handling here

It seems like the easiest solution might be to remove a performance optimization and always call getContentSigningKey when calling identity. We can also consider e.g. having the emitter provide updates of it, but not sure if it is worth it.

authMetadataPromiseRef is typed as ?Promise which means that awaiting it could give us null. One way of avoiding it might be to implement https://linear.app/comm/issue/ENG-6401/introduce-a-hook-that-works-like-useref-and-allows-being-initialized which should hide this detail

type CommServicesAuthMetadata = {
  +userID?: ?string,
  +deviceID?: ?string,
  +accessToken?: ?string,
}

CommServicesAuthMetadata is typed in a way that allows these to be null, but we need these to call authenticated endpoints. A different check would be required when calling an unauthenticated client.

Yeah, it is. I think that throwing might be better here, as it means that a service broke a contract and returned an invalid response.

Sure, I'll rename it

Having authLayer = null will mean that only an unauthenticated client will be initialized. This makes sense in some scenarios. An error should be thrown only when we need to call a method that requires an authenticated client, but we don't have one.

Initially, I tried to do this, but it doesn't work

Cannot call await with Promise.all(...) bound to p because null or undefined [1] is incompatible with null or
undefined [1] in array element of type argument R [2]. [incompatible-call]

     identity-service/identity-service-context-provider.react.js
 [1]   23│     React.useRef<?Promise<{ +userID: ?string, +accessToken: ?string }>>();
         :
       50│       +accessToken: string,
       51│     }>,
       52│   >(async () => {
       53│     const [deviceID, authMetadata] = await Promise.all([
       54│       getContentSigningKey(),
       55│       authMetadataPromiseRef.current,
       56│     ]);
       57│     const userID = authMetadata?.userID;
       58│     const accessToken = authMetadata?.accessToken;
       59│     if (!deviceID || !userID || !accessToken) {

     /private/tmp/flow/flowlib_ea0c1d4f3fb3e484_501/core.js
 [2] 1841│ declare class Promise<+R = mixed> {

This is caused by the fact that authMetadataPromiseRef contains an optional promise, which is required because we want to avoid unnecessary calls to getCommServicesAuthMetadata.

I've tried a couple of solutions, e.g. filtering the authMetadataPromiseRef.current out if it is null, creating promisses array and then passing it to Promise.all, storing an optional value in a promise, but all of the options I've tried caused more problems.

Overall, awaiting them in a sequence shouldn't affect the performance significantly because authMetadataPromiseRef.current, for most of the times, will immediately resolve (it happens some time after it is initially set, or when an emitter broadcasted a value). So awaiting getContentSigningKey would take almost the same time as awaiting all promises. Also, this code starts only getContentSigningKey, because authMetadataPromiseRef.current is already started.

Another solution to this might be to implement https://linear.app/comm/issue/ENG-6401/introduce-a-hook-that-works-like-useref-and-allows-being-initialized. I started writing the solution, but it also appeared to be a lot harder than I expected.

we're missing an await here i think

Good catch, yeah this should be fixed

No, this code is correct. We're returning the promise which can be awaited by the caller.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

Async functions always return a promise. If the return value of an async function is not explicitly a promise, it will be implicitly wrapped in a promise.

So if we return a value from an async function it will be wrapped. But when we return a promise, nothing happens.