toJSON methods don't respect useDate=false

[roboslone]

This leads to precision loss.
I'm using useDate=false, which affects generated objects, but doesn't affect toJSON method.

[stephenh]

Huh, I was a little surprised toJSON doesn't check for useDate, but I guess it makes sense...

Fwiw @roboslone if you could submit a PR for this, that'd be great! Maybe find an integration test that has a parameters.txt with useDate=false in it, and reproduce a toJSON method observing the precision loss that you mention.

Thanks!

[roboslone]

Sure, I'll give this a try.

[kirill-gadzhiev]

I noticed it today too, and it's really strange that no one had spotted it yet

[roboslone]

yarn build:test doesn't work on my machine (macOS, M1) – alpine refuses to execute protoc with weird errors about libc. However, I was able to fix this by changing base image to node:current-slim. I ran all existing tests locally, everything is fine.

I could revert this change after I'm finished with this issue, or I could keep it so more people are able to run tests.

@stephenh What do you think?

[roboslone]

Kept base image change for now, let me know if you want it reverted.

[stephenh]

🎉 This issue has been resolved in version 1.159.3 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[sheinbergon]

Might I argue this a bad/wrong fix for the non-issue?

toJSON() is strictly meant to generate protojson matching json representations.
It's fine if someone wants to keep useDate=false for object representation it's fine, but mostly - a thing of its own

This fix completely breaks the gRPC agenda of any language any platform compatibility, as protojsons can now be ts-proto compatible only

I believe there was a reason why useDate didn't apply to json representations.

We have a JVM/Python/Node JS ecosystem and we used useDate=false, but relied on the json repesentation to be protojson compatible. This fix completey broke everything for us and we need to downgrade the ts-proto version ATM.

I think we should parameterize this option, or create another param (useDateJson=false) to allow for better compatiblity.