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

How do infinity and negative infinity serialize? #33

Closed
sfackler opened this issue Jul 3, 2018 · 9 comments
Closed

How do infinity and negative infinity serialize? #33

sfackler opened this issue Jul 3, 2018 · 9 comments

Comments

@sfackler
Copy link
Member

sfackler commented Jul 3, 2018

The spec indicates that doubles serialize as number | "NaN" - would infinities be treated as NaN then?

@ferozco
Copy link
Contributor

ferozco commented Jul 3, 2018

thats correct

@ferozco ferozco closed this as completed Jul 3, 2018
@sfackler
Copy link
Member Author

sfackler commented Jul 5, 2018

conjure-java does not implement this properly:

    @Test
    public void double_nan_fields_should_be_serialized_as_a_string() throws Exception {
        assertThat(mapper.writeValueAsString(DoubleExample.of(Double.NaN)))
                .isEqualTo("{\"doubleValue\":\"NaN\"}");
    }
org.junit.ComparisonFailure: 
Expected :"{"doubleValue":"NaN"}"
Actual   :"{"doubleValue":"Infinity"}"

@sfackler
Copy link
Member Author

sfackler commented Jul 5, 2018

The current wire format seems to contradict itself - in particular this doesn't hold for infinities:

Every Conjure object (including primitives, complex objects, containers, etc.) is representable as a JSON object such that, intuitively, deserialize(serialize(object)) == object.

@uschi2000
Copy link
Contributor

uschi2000 commented Jul 5, 2018 via email

@sfackler
Copy link
Member Author

sfackler commented Jul 6, 2018

By making a SafeDouble? That seems like it's probably the best approach. Properly controlling the serialization/deserialization of NaN/Infinity consistently in arbitrary languages is going to be a pretty big pain so it'd be great to avoid the issue entirely.

@markelliot
Copy link
Contributor

Pretty sure we discussed this a month ago and the plan was to:

  • say that Conjure's double does not support NaN or +/-Infinity
  • if necessary, add a union type called ieeedouble or something similar that is a union over Conjure's double, NaN, +Infinity, -Infinity

@uschi2000
Copy link
Contributor

Yes, that ("does not support") is what I was advocating for above.

@dphilipson
Copy link

I agree with @markelliot and @uschi2000 and would like to see this changed. The current behavior of typing doubles as number | "NaN" in TypeScript forces consumers to add a lot of boilerplate, and I expect that almost nobody wants to define APIs which accept or produce NaN values. It's not even safe under the current behavior as pointed out in this thread.

@iamdanfox
Copy link
Contributor

iamdanfox commented Aug 7, 2018

@dphilipson I've tried to capture the tradeoffs of just banning these things in the Handling double NaN and Infinity RFC... we found that the assertion "nobody wants to define APIs which accept or produce NaN values" is not quite true, as there is a plausible use case for app which render customer data, which may itself contain NaN/Infinity.

If your specific feedback is focussed on the typescript ergonomics of doubles then I think there's a typescript specific solution which is to introduce a serialization/deserialization layer so that the JSON string "NaN" received from the wire is translated into the JavaScript NaN value.

EDIT - I've opened palantir/conjure-typescript#48 to describe this proposal in more detail.

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

No branches or pull requests

6 participants