Page MenuHomePhabricator

[web] Update gRPC-web generated files
ClosedPublic

Authored by bartek on Nov 28 2023, 5:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 8:22 PM
Unknown Object (File)
Fri, Nov 1, 2:22 AM
Unknown Object (File)
Wed, Oct 23, 6:32 AM
Unknown Object (File)
Wed, Oct 23, 5:24 AM
Unknown Object (File)
Mon, Oct 21, 1:21 AM
Unknown Object (File)
Mon, Oct 21, 12:23 AM
Unknown Object (File)
Sat, Oct 19, 4:01 PM
Unknown Object (File)
Sat, Oct 19, 3:34 PM
Subscribers

Details

Summary

Updated gRPC-web code generated files to match changes made in this stack up to this diff.

Most of the changes were related to moving RPCs from one proto to the other, so I approached this by opening existing Flow files side-by-side with newly-generated TypeScript files and comparing structural changes (e.g. added/moved types).
Because most of the changes in the prior stack are simple movements between protos, some definitions were possible to be moved here too with only minor changes.

Full list of changes:

  • Added FindUserID proto, with request and response types
  • Deleted UploadOneTimeKeys and its types from unauth proto (it was previously in both protos)
  • Moved Delete and Logout RPCs to the auth proto
    • The Request objects are not necessary so replaced them with Empty (contents of former requests are now in auth metadata)
  • Moved GetOutboundKeysForUser to the auth proto.
    • Partially the types were already there because of keyserver keys endpoints
  • Moved GetInboundKeysForUser to the auth proto along with its types, though they internally use PreKey and IdentityKeyInfo from the unauth proto
  • Moved UpdateUserPassword RPCs along with its types to the auth proto

Depends on D10053

Test Plan

Ran flow to make sure there are no errors.
Also repeated test plan of D9792

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
ashoat requested changes to this revision.Nov 28 2023, 1:10 PM

Most of the changes were related to moving RPCs from one proto to the other, so I approached this by opening these files side-by side and moving flow definition between files

Did you make sure the ordering was the same as the TypeScript files? It's important that we try to keep our Flow definitions here minimally different from the TypeScript definitions. The more "different" and "custom" of an approach you take, the harder it becomes to compare the codegenned TypeScript types with our existing in-repo Flow types. This will make changes more difficult in the future.

Relatedly, it looks like this:

logOutUser(
  request: identityStructs.LogoutRequest,
  metadata: grpcWeb.Metadata | void,
  callback: (err: grpcWeb.RpcError,
             response: identityStructs.Empty) => void
): grpcWeb.ClientReadableStream<identityStructs.Empty>;

Got replaced with this:

logOutUser(
  request: identityStructs.Empty,
  metadata?: grpcWeb.Metadata
): Promise<identityStructs.Empty>;

Can you explain why you made these changes? I have a strong preference to revert these changes in favor of keeping the Flow types minimally different from the TypeScript types.

Sidenote – you're making it rather hard on your reviewer by simultaneously moving and changing code. I have to compare each thing side-by-side manually, which takes a lot of time. Please see here

This revision now requires changes to proceed.Nov 28 2023, 1:10 PM

Most of the changes were related to moving RPCs from one proto to the other, so I approached this by opening these files side-by side and moving flow definition between files

Did you make sure the ordering was the same as the TypeScript files? It's important that we try to keep our Flow definitions here minimally different from the TypeScript definitions. The more "different" and "custom" of an approach you take, the harder it becomes to compare the codegenned TypeScript types with our existing in-repo Flow types. This will make changes more difficult in the future.

Yes, the ordering is the same in TypeScript and Flow files - obviously keeping order consistent also simplifies my work. By saying side by side I also meant comparing them with typescript files, I should've mentioned that in the description, I admit it isn't clear enough.

Relatedly, it looks like this:

logOutUser(
  request: identityStructs.LogoutRequest,
  metadata: grpcWeb.Metadata | void,
  callback: (err: grpcWeb.RpcError,
             response: identityStructs.Empty) => void
): grpcWeb.ClientReadableStream<identityStructs.Empty>;

Got replaced with this:

logOutUser(
  request: identityStructs.Empty,
  metadata?: grpcWeb.Metadata
): Promise<identityStructs.Empty>;

Can you explain why you made these changes? I have a strong preference to revert these changes in favor of keeping the Flow types minimally different from the TypeScript types.

This is intentional - in D9931 I have removed LogoutRequest because it's not needed for the auth proto. The same applies to DeleteUserRequest.

Sidenote – you're making it rather hard on your reviewer by simultaneously moving and changing code. I have to compare each thing side-by-side manually, which takes a lot of time. Please see here

This could be split into three diffs but it will keep the cjs files inconsistent with ts/flow definitions between these diffs.


Full list of changes:

  • Added FindUserID proto, with request and response types
  • Deleted UploadOneTimeKeys and its types from unauth proto (it was previously in both protos)
  • Moved Delete and Logout RPCs to the auth proto
    • The Request objects are not necessary so replaced them with Empty (contents of former requests are now in auth metadata)
  • Moved GetOutboundKeysForUser to the auth proto.
    • Partially the types were already there for keyserver keys endpoints
  • Moved GetInboundKeysForUser to the auth proto along with its types, though they internally use PreKey and IdentityKeyInfo from the unauth proto
  • Moved UpdateUserPassword RPCs along with its types to the auth proto

This is intentional - in D9931 I have removed LogoutRequest because it's not needed for the auth proto. The same applies to DeleteUserRequest.

I'm disappointed that you didn't explain your changes to metadata and callback. I am forced to decide between requesting changes again (for the exact same reason as last time), or manually checking out your diff and running the commands myself to see if the changes are manual, opinionated edits you've made, or some curious behavior of the codegen.

To repeat my feedback from the previous review: you are making it too difficult on your reviewer. This diff should have been split into multiple diffs from the start, and all changes should have been explained inline (especially after I asked). Forcing your reviewer to check out your code to answer a question they've already asked is not a good pattern.

manually checking out your diff and running the commands myself to see if the changes are manual, opinionated edits you've made, or some curious behavior of the codegen.

I did this. It turns out that this is "curious behavior of the codegen". It's not totally clear to me why it sometimes opts for metadata?: grpcWeb.Metadata and sometimes for metadata: grpcWeb.Metadata | undefined... I suppose the latter is selected when there is another param (callback) since an optional param can't be used.

This revision is now accepted and ready to land.Nov 29 2023, 11:07 AM

Relatedly, it looks like this:

logOutUser(
  request: identityStructs.LogoutRequest,
  metadata: grpcWeb.Metadata | void,
  callback: (err: grpcWeb.RpcError,
             response: identityStructs.Empty) => void
): grpcWeb.ClientReadableStream<identityStructs.Empty>;

Got replaced with this:

logOutUser(
  request: identityStructs.Empty,
  metadata?: grpcWeb.Metadata
): Promise<identityStructs.Empty>;

This isn't correct, actually. There are two clients -- a "callback" client (IdentityClientServiceClient) and a "promise" client (IdentityClientServicePromiseClient). When he moved the "callback" client method from the unauthenticated client to the authenticated client, this is what changed:

logOutUser(
  request: identityStructs.LogoutRequest,
  metadata: grpcWeb.Metadata | void,
  callback: (err: grpcWeb.RpcError,
             response: identityStructs.Empty) => void
): grpcWeb.ClientReadableStream<identityStructs.Empty>;

became

logOutUser(
  request: identityStructs.Empty,
  metadata: grpcWeb.Metadata | void,
  callback: (err: grpcWeb.RpcError,
             response: identityStructs.Empty) => void
): grpcWeb.ClientReadableStream<identityStructs.Empty>;

as @bartek explained in his last comment and in the updated description, we don't need identityStructs.LogoutRequest since the data in that struct is now being passed as auth metadata.

in the promise client:

logOutUser(
  request: identityStructs.LogoutRequest,
  metadata?: grpcWeb.Metadata
): Promise<identityStructs.Empty>;

became

logOutUser(
  request: identityStructs.Empty,
  metadata?: grpcWeb.Metadata
): Promise<identityStructs.Empty>;

Thanks! Apologies for the misreading.

This revision was automatically updated to reflect the committed changes.