Skip to content
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

Seamless Timestamp/Date encoding/decoding #1990

Open
glenjamin opened this issue Dec 17, 2021 · 3 comments
Open

Seamless Timestamp/Date encoding/decoding #1990

glenjamin opened this issue Dec 17, 2021 · 3 comments

Comments

@glenjamin
Copy link

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 #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 Automatically wrapping & unwrapping wrappers.proto types? protobufjs/protobuf.js#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/Implement wrapper for google.protobuf.Timestamp, and correctly generate wrappers for static target. protobufjs/protobuf.js#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
Copy link
Author

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

@murgatroid99
Copy link
Member

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.

@glenjamin
Copy link
Author

Yeah, that makes a lot of sense to me.

I'll look into taking this up with them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants