-
Notifications
You must be signed in to change notification settings - Fork 544
Long value incorrectly decoded on Scala.js #393
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
Comments
Thanks @tindzk—I'll take a closer look at this, but at a glance I think this is what you're seeing:
Could you try |
Thanks! I tried the following code: import io.circe._
import io.circe.parser._
println(767946224062369796L)
println(decode[Long]("767946224062369796"))
println(decode[JsonNumber]("767946224062369796"))
println(decode[JsonNumber]("767946224062369796").map(_.toLong)) Here is the JVM output:
Unfortunately, it behaves differently on Scala.js:
|
The problem is that circe uses > JSON.parse("767946224062369796")
767946224062369800
> JSON.parse("767946224062369796").toFixed()
"767946224062369792"
> JSON.parse("767946224062369796").toPrecision()
"767946224062369800" I will make a PR to document the current behaviour. The only way to fix this would be to include a "custom" JSON parser for Scala.js and let users choose between performance or correctness. |
@jonas @tindzk I've just been thinking about this in conjunction with #476. The idea would be that the fallback string decoder for numeric types would use circe's own number parser. That way Scala.js users would get the default JavaScript JSON parsing for ordinary JSON numbers, but if they wanted more precision they could represent their numbers as strings. To me this seems less invasive than entirely changing the way JSON numbers are handled natively in JavaScript. The inconsistency isn't ideal, but I think that's part of the price we have to pay to work on a platform without nice numbers. (Not that the JVM is that much better. 😄). |
Yes, that is a good point. So some kind of |
OK, will there be an option to turn it off when interoperability is needed? |
@jonas When would that be necessary? Just to clarify, the decoding of |
I'm not sure whether having custom behaviour in the decoding process is a good idea. I am concerned it will impact interoperability with other JSON libraries. Wouldn't it be better not to create invalid JSON in the first place? I suppose the standard imposes restrictions on the precision of numbers. So, |
@tindzk The issue is that the JSON standard doesn't put any restrictions on precisions—a JSON number can be as precise as you can fit in a string in memory (or more, I guess). The restrictions come from JavaScript, which in many cases isn't involved as a platform at all when working with JSON. The behavior proposed in #476 doesn't customize JavaScript's JSON parser on Scala.js—it's entirely about decoding, and in my experience it's in line with other libraries in this respect. Representing numbers as strings when you need a lot of precision isn't uncommon—Jackson does it, other JavaScript stuff does it, etc. In circe I've done my best to make this unnecessary, and to make interoperability with libraries that do choose that route as easy as possible, and I think this is just an extension of that effort for Scala.js. |
I see. Thanks for the clarifications. As @jonas suggests, an option for disabling this behaviour would still be desirable. I imagine the case where a developer wants to parse JavaScript-generated JSON in Circe. The decoding process should fail if Circe sees a string instead of a number. |
@tindzk Would you mind opening a separate issue for that? Since the beginning, circe (like Argonaut) has silently fallen back to attempting to parse JSON strings as numbers during decoding for numeric types—#485 just makes this a little more robust (and more consistent on the JVM side). You could do the extra validation you propose right now with |
Thanks for fixing the bug! I created an issue as requested. |
For anyone running into this issue, as of v0.14.2 you can use the cross-platform I hope this might become the default parser in the future, see: |
The following test case fails on Scala.js:
The text was updated successfully, but these errors were encountered: