fix: regression in being able to return a Date as a GRPC return value #534
Conversation
fizx
changed the title
There was a problem hiding this comment.
@fizx sorry for the delayed review, but I think this lgtm.
Yeah, figuring up the .bin files would be great; fwiw let me know if that's just a pita and I can also locally checkout/copy this branch, fix up those bins, and then merge on my side of things, if that's more pragmatic than figuring out root case on that for now.
Thanks!
src/generate-services.ts
Outdated
|
|
||
| const params = [...(options.context ? [code`ctx: Context`] : []), code`request: ${inputType}`]; | ||
| const maybeCtx = options.context ? 'ctx,' : ''; | ||
|
|
||
| let encode = code`${rawInputType}.encode(request).finish()`; | ||
| let decode = code`data => ${outputType}.decode(new ${Reader}(data))`; | ||
|
|
||
| console.error(); |
|
|
||
| const params = [...(options.context ? [code`ctx: Context`] : []), code`request: ${inputType}`]; | ||
| const maybeCtx = options.context ? 'ctx,' : ''; | ||
|
|
||
| let encode = code`${rawInputType}.encode(request).finish()`; | ||
| let decode = code`data => ${outputType}.decode(new ${Reader}(data))`; | ||
|
|
||
| console.error(); | ||
| if (options.useDate && rawOutputType.toString().includes('Timestamp')) { |
There was a problem hiding this comment.
Let's check == .google.protobuf.Timestamp instead of just an includes, just in case there are other have-Timestamp-in-their-name messages in a user's schema.
|
|
||
| console.error(); | ||
| if (options.useDate && rawOutputType.toString().includes('Timestamp')) { | ||
| decode = code`data => ${utils.fromTimestamp}(${rawOutputType}.decode(new ${Reader}(data)))`; |
There was a problem hiding this comment.
Ah yeah... this makes sense, and I see what you mean by this is a slippery slope to just re-coding ~all of our well known/use date/etc. special type handling that we currently have ~quite a few of here:
https://github.com/stephenh/ts-proto/blob/main/src/main.ts#L839
Yeah, it'd be great to re-use code, but dunno, I'm pretty comfortable with what you have as a point/incremental solution to a use case that's covered by a test case, and we will magically refactor things at some point in the future (...probably... :-)).
|
Now that we're trued up in #546, this looks more reasonable. Going to land after the status checks pass. |
## [1.110.3](v1.110.2...v1.110.3) (2022-04-08) ### Bug Fixes * regression in being able to return a Date as a GRPC return value ([#534](#534)) ([22b76ec](22b76ec))
|
🎉 This PR is included in version 1.110.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Great, thanks for pushing this through @fizx ! |
The above will not respect ts_opt useDate=true. I know it's more idiomatic to wrap the response in a message type, but bear with me--it's legal, used to work fine, and is a pretty terse, relevant interface.
This PR fixes this, adds a regression test. I do feel like the fix is clunky, and likely doesn't address every instance of similar problems. Some advice there would be nice.
Also, I don't really know why my M1 laptop insists on making the following 1 chr diff to random *.bins, but it doesn't seem to be affecting anything. I'll investigate that separately: