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: Non-integers were always parsed as strings, if storeAsString #89

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

Conversation

edgarsi
Copy link

@edgarsi edgarsi commented Sep 13, 2023

Losing precision after the decimal point is almost always a non-issue. Using 'string' in such cases is usually worse.

Losing precision after the decimal point is almost always a non-issue.
Using 'string' in such cases is usually worse.
@edgarsi
Copy link
Author

edgarsi commented Sep 14, 2023

To be frank, it isn't clear to me what the purpose of storeAsString is:

  • If may be to retain full information. In this interpretation, my pull request is wrong.
  • It may be to let the any parsed result x to be stringified without data loss, e.g. int64 identifiers for UI output. This was my concern.

If json-bigint is used as a drop-in replacement for parsing all jsons, it is safe to say we don't want to convert to string in cases where it isn't necessary, as it may break components expecting non-string types. There are several questionable cases:

  • Integers larger than 2^53 - we do want to convert. Otherwise, why is json-bigint even used?
  • Non-integers larger than 2^53 - ?. I am converting them, because they were converted prior to this PR, so converting creates the least amount of breaking change, while letting json-bigint claim "No integer part will ever be damaged by parsing". I don't know any use case for such numbers though, and have heard an opinion that non-integers should always be safe to round.
  • Scientific notation - ?. To me, notation does not matter, only the number itself. But prior to this PR, any large numbers in scientific notation were parsed as number, including large integers. I am not changing that, though I disagree with the choice.

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.

1 participant