Add ProtoJS types to generated proto types #2749
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/protochanges
branch
2 times, most recently
from
5c742e7 to
69e5386
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/protochanges
branch
from
69e5386 to
b887735
Compare
| nanos?: number; | ||
| } | ||
| // Denotes the possible representations for timestamps in the Value type. | ||
| export type TimestampValue = |
There was a problem hiding this comment.
The Value proto can contain a timestamp and the name TimestampValue suggests that this is one of those, when it's really just the timestamp itself. Also, this is somewhat confusing because historically FooValue has been one of our model types and not a proto. In this regard the old TimestampProto name was better.
There was a problem hiding this comment.
I renamed it to Timestamp, which aligns with LatLng and GeoPoint. I also moved it to the proto d.ts file.
| @@ -148,46 +146,33 @@ export class JsonProtoSerializer { | |||
| * our generated proto interfaces say Int32Value must be. But GRPC actually | |||
| * expects a { value: <number> } struct. | |||
| */ | |||
| private toInt32Value(val: number | null): number | null { | |||
| if (this.options.useProto3Json || typeUtils.isNullOrUndefined(val)) { | |||
| private toInt32Value(val: number | null): number | { value: number } | null { | |||
There was a problem hiding this comment.
Optional: toInt32Value doesn't really do much to indicate that it's converting to a proto message. What do you think of renaming these (and related items) to names like toInt32Proto?
The Android/iOS convention of naming these encode/decode helps somewhat.
There was a problem hiding this comment.
I renamed this instance to toInt32Proto but did not convert the methods to the encode/decode pattern. I am personally also confused by the naming in the file and constantly try to use the wrong methods (toValue instead of fromValue etc)
| @@ -180,7 +180,7 @@ export declare namespace firestoreV1ApiClientInterfaces { | |||
| name?: string; | |||
| fields?: ApiClientObjectMap<Value>; | |||
| createTime?: string; | |||
| updateTime?: string; | |||
| updateTime?: string | { seconds?: string | number; nanos?: number }; | |||
There was a problem hiding this comment.
Is this generated or hand edited?
If hand edited, would it be worthwhile to define the timestamp type in here and reuse it for all these declarations?
Then we could just call it Timestamp and refer to it as api.Timestamp.
There was a problem hiding this comment.
This used to be generated, but has been hand edited many times since then. As such, I added api.Timestamp.
See #2715 for the bigger picture.