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

fix(type): Correctly map proto to json according to spec #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paulip1792
Copy link

In protobuf definition (https://developers.google.com/protocol-buffers/docs/proto3#json), 64 bits numeric types should be interpreted as string in JSON.

@paulip1792 paulip1792 requested a review from ericwenn as a code owner August 4, 2022 07:25
@ericwenn
Copy link
Member

ericwenn commented Aug 4, 2022

Hello @paulip1792, thanks for contributing.
I ran the CI pipeline, and youre missing the changes to generated code in your commit.

For my understand have you seen any isues with the current mapping in your applications?

@paulip1792
Copy link
Author

paulip1792 commented Aug 4, 2022

@ericwenn I am using protojson (https://pkg.go.dev/google.golang.org/protobuf/encoding/protojson) to marshal the response and 64 bits integer will be quoted as string value. I guess it's because JS Number can only represent 53 bits integer, so it's defined as string in the spec. In my opinion, in order to make sure it's all type-safe, 64 bits integer should be represented in string. I will make the CI happy if it is accepted change since I know it's a breaking change.

@ericwenn
Copy link
Member

ericwenn commented Aug 4, 2022

I agree we should address this.
As it turns out we rarely use int64 in our protobuf schemas, so that is likely why we have not noticed it.

Thanks for the PR and reading through the spec!

@ericwenn
Copy link
Member

@paulip1792
Is this still something you want to work on fixing?

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

Successfully merging this pull request may close these issues.

2 participants