grpc-node icon indicating copy to clipboard operation
grpc-node copied to clipboard

Seamless Timestamp/Date encoding/decoding

Open glenjamin opened this issue 3 years ago • 3 comments

Is your feature request related to a problem? Please describe.

Currently dealing with google.protobuf.Timestamp types is rather clunky if you want to interop native JS Date instances. Custom code must be used upon reading and writing messages to translate.

Describe the solution you'd like

Introduce an option similar to longs and bytes which enables the generated code and types to prefer Date instances for Timestamp-typed fields.

Implementation-wise my suggestion is as follows:

  • Add a new option called timestamp (or google.protobuf.Timestamp) which accepts the value Date
  • When enabled, the generated types would use Date for any timestamp fields
  • When enabled, the protobufjs-wrapping code would override the default toObject and fromObject on the Timestamp class to accept dates

If/when protobufjs gains an option to do native Dates, the third bullet could be replaced with that (or I could try and get something landed with them first, before pursing the type-generation change here).

I'm happy to implement this myself and submit a PR, provided I a bit of steer on whether this would go anywhere.

Describe alternatives you've considered

I note that this was previously asked for in https://github.com/grpc/grpc-node/issues/357 and closed, but the ecosystem has progressed since then.

I've done a bunch of code spelunking today and here's what I think the current state of things is:

  • protobufjs v6 gained support for custom wrappers, which can extend the default toObject/fromObject implementations (mentioned in https://github.com/protobufjs/protobuf.js/issues/677)
  • the custom wrappers are defined on the protobufjs module globally, and aren't directly exposed by proto-loader
  • it is also possible to implement by augmenting the methods of the generated Timestamp class, but this also isn't directly exposed by proto-loader (they're on the protobufjs root).
  • There is an old protobuf.js PR to add a wrapper native Date support, but this appears to have stalled and it's focused on JSON encoding anyway [https://github.com/protobufjs/protobuf.js/pull/1258]
  • Even if that PR was responded to by maintainers and landed, returning Dates from toObject would likely need to be opt-in to keep remain backwards compatible
  • proto-loader's typescript generation code would also need a flag to modify the generated types, fromObject could be backwards compatible and accept either the seconds/nsec object or Date, but toObject has to pick a side.

If there's another way to make this all interop nicely with less work, or without making changes to core I'd be happy to take that approach instead!

glenjamin avatar Dec 17 '21 17:12 glenjamin

Any thoughts on a way forwards here? Or should I be starting with a petition to protobuf.js?

glenjamin avatar Feb 21 '22 18:02 glenjamin

I'm sorry for not responding earlier. I saw this but it dropped off my radar over Christmas. My main concern here is that proto-loader is currently a mostly-transparent wrapper around protobufjs, especially in terms of serialization and deserialization behavior. With this change, it would start to have its own serialization behavior and options, which would increase complexity and the risk of future incompatibilities with protobufjs.

murgatroid99 avatar Feb 23 '22 22:02 murgatroid99

Yeah, that makes a lot of sense to me.

I'll look into taking this up with them

glenjamin avatar Feb 24 '22 07:02 glenjamin