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

Throw when JSON.stringify works with decimals #167

Merged
merged 9 commits into from
Sep 17, 2024
Merged

Conversation

jessealama
Copy link
Collaborator

As a first step in the integration between JSON and decimals, let's make decimal align with BigInts by throwing a TypeError.

This decision isn't necessarily final. The change is made so that, at least, we have some kind of defined behavior. We may end up returning a value instead of throwing.

CC @sffc

@jessealama jessealama linked an issue Jul 24, 2024 that may be closed by this pull request
jessealama and others added 2 commits July 25, 2024 10:13
Remove special logic in the `SerializeJSONProperty` AO.
@sffc
Copy link

sffc commented Jul 25, 2024

Seems okay, though if we do this then the urgency of toString returning the full data model (#12) is elevated, since developers will reach for toString when serializing to JSON.

@safinaskar
Copy link

Of course, JSON.stringify should convert decimals to string. I. e. if we have decimal embedded in some object, then JSON.stringify should return something like {"foo":"0.1"}. Because this is common practice to represent monetary decimals in REST APIs as strings

@sffc
Copy link

sffc commented Jul 28, 2024

I would be slightly in favor, I think, of Decimal supporting toJSON, but if it does, it should retain the whole data model (trailing zeros) in order for it to round-trip.

@jessealama jessealama merged commit d884464 into main Sep 17, 2024
@jessealama jessealama deleted the 166-tojson-is-missing branch September 17, 2024 14:14
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.

toJSON is missing
5 participants