ts-proto icon indicating copy to clipboard operation
ts-proto copied to clipboard

feat: support unwrapped value type rpc method arguments

Open programmador opened this issue 1 year ago • 5 comments

This part only adds support of unwrapped arguments, not return values. Currently I do not have any value-type return types in my projects so I don't have an ability to check whether generated code does work if it contains such return values. So this specific PR contains only stable code which I was able to check in a real project.

Either I'll create another PR when unwrapping of value-type return values is supported or just add commits to this one later.

programmador avatar Dec 21 '23 12:12 programmador

Here's the link for prelude conversation #978

programmador avatar Dec 21 '23 12:12 programmador

Seems like there's an interface trouble though somehow the code is being compiled and working in my project. Fixing an interface gives strange wrappers-regression.ts change which causes a test failure:

 export interface Clock {
   Now(request: Empty): Promise<Timestamp>;
-  NowString(request: StringValue): Promise<StringValue>;
-  NowStringStream(request: Observable<StringValue>): Observable<StringValue>;
+  NowString(request: string | undefined): Promise<StringValue>;
+  NowStringStream(request: string | undefined): Observable<StringValue>;
   NowBool(request: Empty): Promise<BoolValue>;
 }

It seems like the stream case is not covered correctly and I'm not sure whether it could be fixed at grpc-web level. Maybe types.ts changes are needed to implement rpc-level support for wrappers instead.

programmador avatar Dec 21 '23 13:12 programmador

Now I've moved the value type related branching lower into types.ts. Unfortunately it involved creating one more boolean flag which is quite tricky to reason about at upper levels (grpc-web & service). In particular the code fragment || methodDesc.clientStreaming disables new functionality for interface generation of streaming methods for the same reason this PR doesn't include any changes for rpc return values: I just have no idea where to test it. Just looking at generated code diff is not enough to be sure it works as intended. Moreover grpc-web implementation just throws an exception for streaming methods.

programmador avatar Dec 21 '23 14:12 programmador

The only problem still persists: incompatibility of string | undefined with non-optional StringValue.value. It seems like messageToTypeName in types.ts does not respect that option at all and just always adds that | undefined, am I right?

programmador avatar Dec 21 '23 16:12 programmador

Trying to implement smth like:

    const undefinedPrefix = ctx.options.useOptionals && ctx.options.useOptionals != 'none'
      ? ' | undefined'
      : '';
    return code`${valueType}${undefinedPrefix}`;

instead of just

return code`${valueType} | undefined`;

inside of messageToTypeName() breaks just everything.

From this point I'm actually not sure how it's meant to be. From one side I've broken everything, from the other side why do we put that | undefined everywhere when optionals are disabled?

programmador avatar Dec 21 '23 16:12 programmador