-
Notifications
You must be signed in to change notification settings - Fork 345
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
fix: code-generation for Services with Struct response types #407
Conversation
Thanks for the swift response @boukeversteegh ! I tested it on the https://github.com/mkmarek/ts-proto-bug repo and it worked like a charm. So I tried to use it on the project where I got the error originally and noticed we're using a bit different configuration there. We use it with this parameter Example: export const ServiceService = {
foo: {
path: '/pkg.Service/Foo',
requestStream: false,
responseStream: false,
requestSerialize: (value: FooRequest) =>
Buffer.from(FooRequest.encode(value).finish()),
requestDeserialize: (value: Buffer) => FooRequest.decode(value),
// ↓↓↓↓↓
responseSerialize: (value: {[key: string]: any} | undefined) =>
Buffer.from({[key: string]: any} | undefined.encode(value).finish()),
responseDeserialize: (value: Buffer) => {[key: string]: any} | undefined.decode(value),
}
} I updated https://github.com/mkmarek/ts-proto-bug with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm with @mkmarek 's caveat that we probably need another "keepValueTypes" (I assume) somewhere.
@boukeversteegh could you add an RPC that returns a struct to this file:
https://github.com/stephenh/ts-proto/blob/main/integration/grpc-js/simple.proto#L27
As part of the fix, to prevent future regressions?
@@ -573,7 +573,7 @@ export function requestType(ctx: Context, methodDesc: MethodDescriptorProto): Co | |||
} | |||
|
|||
export function responseType(ctx: Context, methodDesc: MethodDescriptorProto): Code { | |||
return messageToTypeName(ctx, methodDesc.outputType); | |||
return messageToTypeName(ctx, methodDesc.outputType, { keepValueType: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's subtle; thanks @boukeversteegh !
Added that case, and fixed some issues exposed by it as well. I'll also add a case for |
dfed825
to
2727e3f
Compare
2727e3f
to
cf4ba75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I really like where you put the unwrap
methods on each type:
Value.unwrap(Value.decode(reader, reader.uint32()));
I think let's merge this as-is; just as a curiosity, seeing ^ made me wonder, @boukeversteegh wdyt about having Value/Struct/List.decode
return the already-unwrapped values? And similarly, Value/Struct/List.encode
could accept the not-yet-wrapped values (or maybe if we're really fancy, encode
could accept a union of either Value | ...the unwrapped values...
?
Just thinking that might be neat for users b/c it would make the wrapper/value types "fade away" to be mostly serialization. Well, although I guess our message representation is already doing that, i.e. if you have a Struct
inside another message, it already looks like "just JS", and it's only the encode
/decode
methods that want/need to know about the wrapped Struct
type.
And so my musing of changing decode
to return the already-unwrapped type is probably not something that users would use/notice that often anyway, b/c they're more likely interacting with a value type through some message that uses it. So maybe it's better to stay consistent and have encode
/decode
keep using the wrapper/message types, just like all of the other encode
/decode
methods do.
I had the exact same train of thought, and ended up with the same tentative conclusion, to keep the signatures of the encode/decode methods the same as the others. Two other reasons why I felt it might be better to keep the wrapping separate from the encoding:
Great, thanks for your review and support. I will go ahead and merge! |
## [1.90.1](v1.90.0...v1.90.1) (2021-11-27) ### Bug Fixes * code-generation for Services with Struct response types ([#407](#407)) ([f041fa1](f041fa1))
🎉 This PR is included in version 1.90.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Should fix #403
Could you have a try? @mkmarek -- and thanks for reporting with a reproducing proto file.
You can switch to this version temporarily by changing the version in your package.json to: