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

Encode int64 value as string in json codec #231

Merged
merged 2 commits into from
Feb 6, 2022

Conversation

zindel
Copy link
Contributor

@zindel zindel commented Jan 7, 2022

Currently, atdgen-runtime encodes int64 value as JSON number. This makes it difficult to use the resulting JSON in the JavaScript environments (node, browser). For example, consider this issue with the Int64.max_int value:

Welcome to Node.js v12.16.2.
Type ".help" for more information.
> JSON.parse("9223372036854775807")
9223372036854776000

Right away, some information is lost. If we encode it as string though, we don't have such problem anymore. Moreover decoding already supports string as the transportation format for int64 (test is added). This change fixes the interoperability with bucklescript ahrefs/melange-atdgen-codec-runtime#22

Nevertheless, the change is breaking, so version bump is needed if merged.

@rr0gi
Copy link

rr0gi commented Jan 7, 2022

change is breaking in the sense that on-the-wire json is changed?

@zindel
Copy link
Contributor Author

zindel commented Jan 7, 2022

@rr0gi yes. Before 123, after "123"

@zindel zindel merged commit f18858b into master Feb 6, 2022
@zindel zindel deleted the zindel/json-write-int64-string branch February 6, 2022 08:04
@Khady
Copy link
Contributor

Khady commented Feb 7, 2022

this needs a mention in CHANGES.md

@AndreyErmilov
Copy link

@Khady @zindel You forgot to mention this change in CHANGES.md :)

@mjambon
Copy link
Collaborator

mjambon commented Apr 4, 2022

Sorry, I made the release (2.3.0) without trying hard to update the changelog. I think it's still a good idea to update it after the fact. You should be able to update CHANGES.md now in git, and edit manually the release page https://github.com/ahrefs/atd/releases/tag/2.3.0 (copy-paste the 2.3.0 section from CHANGES.md to replace what's already there).

@Khady
Copy link
Contributor

Khady commented Apr 7, 2022

Sorry about that. I've updated the file and the release

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.

6 participants