Skip to content

Add RequestType and ResponseType metadata to method definition #408

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

Closed
wants to merge 2 commits into from

Conversation

Jmoore1127
Copy link

PR for issue #407

@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@Jmoore1127
Copy link
Author

CLA Signed.

@nicolasnoble
Copy link
Member

Different take on #378.

@@ -37,6 +37,8 @@ export interface MethodDefinition<RequestType, ResponseType> {
requestDeserialize: Deserialize<RequestType>;
responseDeserialize: Deserialize<ResponseType>;
originalName?: string;
requestType: RequestType;
responseType: ResponseType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These type declarations aren't really semantically correct. RequestType and ResponseType are the types of objects that are passed to and received from the gRPC methods. In the case of this library, those are plain objects with a structure matching the declared proto message types, which is why the ServiceDefinition just declares them with object, object.

But requestType and responseType are not that. They are Protobuf.js message descriptor objects that describe the structure of RequestType and ResponseType. @nicolasnoble linked another PR that tried to do this and they declared those values as Protobuf.Type, which I think is the actual correct type.

@murgatroid99
Copy link
Member

I want to mention that I have the same concerns I had in #378, but I am willing to reconsider.

@Jmoore1127
Copy link
Author

I am happy to change types when I am back at my desk tomorrow.

I am not picky about how the information is exposed, I just need some way to access it. I can update our tooling in order to parse the structure. My main problem is that the information is captured within closures for the serializers and deserializers so I have no way of accessing it without parsing the file twice (once with grpc and once with protobufjs).

@Jmoore1127
Copy link
Author

@murgatroid99 I have updated the typings to match actual types as suggested. I think I better understand your comment about not exposing protobufjs apis now. Do you have any suggestions as to what you would accept for the requestType and responseType objects? At a minimum, I need information such as field names, field types, and structure (for nested types) in order to create sample request and response objects.

@murgatroid99
Copy link
Member

One option, to make it as general as possible, would be to use the JSON representation of the DescriptorProto (from google/protobuf/descriptor.proto) objects that correspond to the two message types. Unfortunately, it's pretty hard to actually get that representation out of Protobuf.js.

@murgatroid99
Copy link
Member

I am sorry for the long delay here. If you are still interested in this problem, I have proposed a solution at grpc/proposal#116. Please feel free to comment on it.

@murgatroid99
Copy link
Member

Thank you for your contribution. This change has been superseded by #703.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants