Page MenuHomePhabricator

[keyserver] add flow types for grpc client
ClosedPublic

Authored by varun on Nov 10 2022, 3:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 4, 12:37 AM
Unknown Object (File)
Mon, Nov 4, 12:37 AM
Unknown Object (File)
Mon, Nov 4, 12:35 AM
Unknown Object (File)
Mon, Nov 4, 12:34 AM
Unknown Object (File)
Mon, Nov 4, 12:24 AM
Unknown Object (File)
Fri, Oct 25, 5:18 PM
Unknown Object (File)
Fri, Oct 25, 5:18 PM
Unknown Object (File)
Fri, Oct 25, 5:18 PM

Details

Summary

adding the relevant types for the grpc client. I had to use any/mixed a few times to avoid pulling in more dependencies like the events library and http2.

Test Plan

ran flow in keyserver
also, after the subsequent diff, I added the following code to keyserver.js and re-ran flow:

+  const user = { userID: '12345', deviceID: 'ahhhhh' };
+  const publicKey = identityClient.getUserPublicKey(user, (e, o) => {
+    if (e) {
+      console.log(e);
+    } else {
+      console.log(o);
+    }
+  });
+  console.log(publicKey);

also ran yarn dev and got back the expected public key

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun held this revision as a draft.
atul added a reviewer: ashoat. atul removed 1 blocking reviewer(s): atul.Nov 14 2022, 12:42 PM

Adding @ashoat as blocking because flow

ashoat requested changes to this revision.Nov 14 2022, 5:27 PM
  1. Should this be defined as a libdef (eg. in a declare module "package_name" { declaration) inside the flow-typed folder?
  2. I'd like to replace the export interface declarations with more idiomatic ones, but it's hard for me to test this outside of your stack. Maybe we try together in the office sometime?
lib/types/grpc-types.js
46 ↗(On Diff #18358)

Based on the name, kinda weird that this doesn't return like a string, but I am not familiar with this API

71 ↗(On Diff #18358)

export interfaces is weird and best to avoid... export type is preferable, but there's a separate construction for functions

126 ↗(On Diff #18358)

Is this perhaps the wrong link?

This revision now requires changes to proceed.Nov 14 2022, 5:27 PM

change interfaces to types and fix toJSON return type

  1. Should this be defined as a libdef (eg. in a declare module "package_name" { declaration) inside the flow-typed folder?
  2. I'd like to replace the export interface declarations with more idiomatic ones, but it's hard for me to test this outside of your stack. Maybe we try together in the office sometime?
  1. I don't think this should be defined as a libdef since some of the types are code-generated and specific to our identity.proto file. That said, I could probably separate the code-generated types from the grpc-js package types... are there any other considerations for deciding where to define things?
  2. Thanks for the help with this
lib/types/grpc-types.js
46 ↗(On Diff #18358)

it should return a map, I was confused by the typescript function signature, which didn't specify a return value https://github.com/grpc/grpc-node/blob/master/packages/grpc-js/src/metadata.ts#L241

71 ↗(On Diff #18358)

thanks for the help with this

126 ↗(On Diff #18358)

it was correct, i shouldn't have any-typed the EventEmitter

I don't think this should be defined as a libdef since some of the types are code-generated and specific to our identity.proto file. That said, I could probably separate the code-generated types from the grpc-js package types... are there any other considerations for deciding where to define things?

Let's get this landed and then maybe you can create a Backlog task for properly typing the libdef and separating out the parts here that are generated and specific to our implementation?

Also before landing, can you add a diff comment showing which parts are from the libdef vs. from our implementation / codegen?

This revision is now accepted and ready to land.Nov 15 2022, 1:22 PM
lib/types/grpc-types.js
3–10 ↗(On Diff #18441)

these are code generated types

135–141 ↗(On Diff #18441)

this is also code-generated. everything else is from the grpc-js npm package and can be defined as a libdef

This revision was automatically updated to reflect the committed changes.