Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support unwrapped value type rpc method arguments #979

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

programmador
Copy link
Contributor

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
Copy link
Contributor Author

Here's the link for prelude conversation #978

@programmador
Copy link
Contributor Author

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
Copy link
Contributor Author

programmador commented Dec 21, 2023

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 programmador changed the title Support unwrapped value type rpc method arguments feat: support unwrapped value type rpc method arguments Dec 21, 2023
@programmador
Copy link
Contributor Author

programmador commented Dec 21, 2023

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
Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant