useDate=false doesn't match protobuf JSON spec about ts-proto HOT 11 CLOSED

toJSON() is strictly meant to generate protojson matching json representations

I didn't realise that at the time.
In my case protojson compatibility does not matter, I just need to store messages in JSON with full precision.

adds a new jsonTimestampPrecision flag that defaults to 3 but can go up to 6/9 to get the precision he wants

I'm not sure this is a good idea. Setting timestamp precision to 6 or 9 will make it incompatible with Date and (i beleive) most other JSON parsers.

adds a different jsonTimestamps flag with values of like rfc3339 or raw

This seems more reasonable to me. Basically raw value would mean "I don't care about protojson compatibility, just store most precise timestamps as objects"; and other (default) value would mean "Keep protojson compatibility, always use Date".
Personally I think default option should be called date, since we would use Date to marshal timestamp value.

from ts-proto.

Hi @roboslone , yep, that sounds good! I'm thinking maybe the new option would be:

• useJsonTimestamp=rfc3339 | raw
• Defaults to rfc3339
• The behavior you want is the raw value
• Maybe eventually we add rfc3339-6 or rfc3339-9 to add more decimals of precision to the output, but still within the ISO string

Thanks!

from ts-proto.

From a first glance I tend to agree that useDate doesn't logically seem to be related to toJson and since toJson is supposed to be protojson compliant, the original implementation of casting to a date and formatting as an iso string makes more sense here.

from ts-proto.

@stephenh would love to hear your thoughts about this. We need to be able to fix this issue.

from ts-proto.

@roboslone given you contributed the "useDate ==> keep seconds + nanos in the JSON" PR (#937 ), can you weigh in on this issue?

My guess is that you have a somewhat adhoc set of protobuf JSON APIs, where you've determined the loss of precision was worth deviating from the protobuf spec (and your/my 1st guess at "keeping more precision" was putting the raw seconds/nanos on the wire)?

More closely reading the spec, it says Timestamps:

Uses RFC 3339, where generated output will always be Z-normalized and uses 0, 3, 6 or 9
fractional digits. Offsets other than "Z" are also accepted.

And ts-proto's pre-937 behavior:

"lastEdited": "1973-11-29T21:33:09.234Z",

It looks like we had been outputting 3 digits of fractional seconds, but the spec allows up to 9 digits, and that probably would have provided @roboslone with the precision he desired?

My inclination is to revert #937 and then ask @roboslone to follow up with a PR that does one of two things:

1. adds a new jsonTimestampPrecision flag that defaults to 3 but can go up to 6/9 to get the precision he wants, but keeps the output being an RFC 3339 string
2. adds a different jsonTimestamps flag with values of like rfc3339 or raw to let him flip into raw mode and get the seconds / nano values on the wire.

My preference would be for 1 given it says spec complaint.

@sheinbergon understood your urgency, so thanks for highlighting but I assume we can wait a few days to give @roboslone a chance to respond, and for now you can just hold off on upgrading.

from ts-proto.

Sure. We can wait it out, even weeks if required. Just wanted to get your attention.

10x !

from ts-proto.

I'll just add that I favour rfc3339 instead of date, as it better reflects the expected outcome, as opposed to code internals.

from ts-proto.

Yeah, okay, you're right.

from ts-proto.

@stephenh I could provide a PR for second option in 3 days and there would be no need for revert. Are you okay with this?

from ts-proto.

This should be fixed/restored in #969 ; thanks for the PR, @roboslone , appreciate your flexibility!

from ts-proto.

🎉 This issue has been resolved in version 1.164.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release
• v1.164.1

Your semantic-release bot 📦🚀

from ts-proto.