we want the client methods to match what we have on native in commRustModule. also, this wrapper class lets us hide some other implementation details that the caller doesn't need to worry about (e.g. version interceptor, auth vs unauthorized client)
Details
Details
- Reviewers
inka tomek ashoat - Commits
- rCOMMe18fe51757c0: [web] add gRPC client wrapper
successfully called local identity service by setting localhost socket address in keyserver/secrets json file
checked that if config is absent, the correct default is used depending on NODE_ENV value
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/grpc/identity-service-client-wrapper.js | ||
|---|---|---|
| 24 ↗ | (On Diff #34408) | I had to add this check because flow expects identityServiceConfigRaw to be a string, but really it's an object |
Comment Actions
we want the client methods to match what we have on native in commRustModule.
Hm, I don't get it. How are we matching what's on Native, if in CommRustModule we have: generateNonce, registerUser, loginPasswordUser, loginWalletUser, updatePassword, deleteUser, getOutboundKeysForUserDevice, versionSupported, and this class has a completely different set of methods?
| web/grpc/identity-service-client-wrapper.js | ||
|---|---|---|
| 24 ↗ | (On Diff #34408) | But is it ever really a string? If not, can't we just have let identitySocketAddr;
if (
typeof identityServiceConfigRaw === 'object' &&
identityServiceConfigRaw !== null
) {
identitySocketAddr = identityServiceConfigRaw.identitySocketAddr;
}
return (
identitySocketAddr ||
(process.env.NODE_ENV === 'development'
? 'https://identity.staging.commtechnologies.org:50054'
: 'https://identity.commtechnologies.org:50054')
);? |
| web/grpc/identity-service-client-wrapper.js | ||
|---|---|---|
| 6–8 ↗ | (On Diff #34409) | We never import cjs files like this. Are you 100% sure that we should do this this way? |
| 44–45 ↗ | (On Diff #34409) | Should we define these in some shared place? In lib/facts we have configs for other services - should we create a new file there for Identity? |
| 24 ↗ | (On Diff #34408) |
Why is that? Is there a way to make Flow correct about the actual type? |
Comment Actions
there are some internal methods here that aren't relevant to commRustModule, but the idea is that this class should be a superset of commRustModule. i've only added deleteUser for now, but will add the other methods later when i work on the related workflows.
| web/grpc/identity-service-client-wrapper.js | ||
|---|---|---|
| 6–8 ↗ | (On Diff #34409) | this is how @ashoat imported these files in his testing patch: https://gist.github.com/Ashoat/7cf4a0fbdebc152d585884184bc0ac07 i can check with him if this is how we want to do it here, though |
| 24 ↗ | (On Diff #34408) | good point, it should never be a string |
| 24 ↗ | (On Diff #34408) | i'm not sure, perhaps flow expects all process.env values to be a string. i'll ask @ashoat |
Comment Actions
Looked into the issue with process.env.IDENTITY_SERVICE_CONFIG showing up as an object. This appears to be due to some quirky behavior of webpack.DefinePlugin... it looks like if it's handed a stringified object, it will expose that as an object in the runtime.
Here are some potential ways to work around it:
- Don't pass a stringified object in, and instead pass in just the identitySocketAddr value
- Override Flow with something like const identityServiceConfigRaw: IdentityServiceConfig = (process.env.IDENTITY_SERVICE_CONFIG: any);
- Replace the conditional with an invariant
Not sure if this would work, but you can try defining this without the nested structure, eg. like this:(this doesn't work)
new webpack.DefinePlugin({ 'process.env': mostEnvVars, 'separateGlobalForIdentityServiceConfig': identityServiceConfig, // stringified object }),
Comment Actions
I'd like one of my three suggestions above to be applied before landing, but won't block the diff right now since my review speed is a little slow
| web/grpc/identity-service-client-wrapper.js | ||
|---|---|---|
| 24–32 | This code looks really scary. If IDENTITY_SERVICE_CONFIG isn't set (undefined), won't this completely crash? | |