Inconsistent Well-Known Types handling with grpc-gateway

[lixin9311]

Previously discussed in #4

Although we should run most other protoc plugins with WKT definitions come with the protoc (timestamp.pb, duration.pb etc.).
In the case of protoc-gen-grpc-gateway-ts, we should generate TypeScript definitions compatible with grpc-gateway's JSON mapping (and protoc-gen-openapiv2).

Currently, by running the protoc-gen-grpc-gateway-ts on timestamp.pb, we will get the underlying definition of the timestamp.

// example message
export type Message = {
  value?: string
  waitPeriod?: GoogleProtobufDuration.Duration
  ts?: GoogleProtobufTimestamp.Timestamp
}
// the definition of GoogleProtobufTimestamp.Timestamp
export type Timestamp = {
  seconds?: string
  nanos?: number
}

Which will be rejected by grpc-gateway.
The correct JSON mapping of the timestamp is a datetime in RFC3339(nano) format, which can be fulfilled by the default behavior of JSON.stringify a Date object in JavaScript.

I think we still need to set up exception rules for parsing those WKTs, at least for Timestamp and Duration.

atreya2011@a1e3d5c seems to be a good patch without the handling of wrappers, duration and etc.

[lyonlai]

alright, looking at the mapping in this page again. looks like timestamp needs to be mapped to proper date string format on the wire.

Ideally putting the Date type in the generated field will be fit. However translating it back from the wire will become the essence of this piece of work. @lixin9311

[atreya2011]

Couldn't we leave the "translating it back from the wire" to the client (end-user using the library)?
On the server-side, grpc-gateway will take care of the translation.
On the client-side, we can let the end-user decide on the translation method.

[lyonlai]

there will be annoying type mismatch if we don't do it in the plugin

[atreya2011]

I see, but I think grpc-gateway parses it for us before sending the response to the client. (https://github.com/grpc-ecosystem/grpc-gateway/blob/24434e22fb9734f1a62c81c4ea246125d7844645/runtime/marshal_proto_test.go)
And we can use annotations in the proto file to specify the marshalling format as well.

Let me know if my understanding is correct 🙏🏼

[lixin9311]

I'm not very good at Typescript. But I think we might have to define a customized type for those, or just leave them as strings for now.

Some refs:
protobufjs/protobuf.js#893
https://github.com/stephenh/ts-proto#timestamps
https://googleapis.dev/nodejs/firestore/3.3.3/Timestamp.html

[lixin9311]

BTW, bytes - Uint8Array is also inconsistent with the default JSON mapping.
It should be a base64 url safe encoded string.

[lixin9311]

I've been trying to figure out a solution for a week. But I see no other way than using class definitions (with customized methods) or extra helper functions instead of just plain types.
Typescript does type check during compile time, and preserved little information in the Javascript code. Therefore, it is hard to access the type information we need at runtime. The same problem also applies to #23.

There could be huge API change if switch to class, and other issues (OneOf implementation).
Currently, I have to map the WKTs and bytes to strings to workaround the problem.

[lyonlai]

well I believe there are still ways to get around this without using class and the keep the simplicity of using POJOs. I'll give it a try later on when I'm freed up a bit more.

[remko]

I don't really understand the reasoning behind the complexity yet. As far as I can tell, this plugin doesn't do any transformations on the JSON, and just passes everything straight through to the client. Why start with this complexity now, instead of just adding a type GoogleProtobufTimestamp = string (which is what the gateway is returning), and let the client deal with turning this to/from a Date?

Not that a Date wouldn't be convenient, but I'm not sure if it's worth the extra complexity. (And break backwards compatibility)

[ashwin153]

@lyonlai Were you able to get this working? Would it be possible to upstream this change cresta@65941a4?

[shaders]

Is this repo still maintained?

[atreya2011]

@lyonlai I can work on this if you give me the general direction. I already made a patch before.

[junm-cloudnatix]

Any updates on this issue? I have the same trouble but with google.protobuf.Any type (which should be any in typescript).