Clarify that Float does not include NaN or infinity #780
Conversation
glasser
force-pushed
the
glasser/finite-floats
branch
from
acf270e
to
df8039e
Compare
|
@juriad is this suggestion directly related to my change, or a separate opportunity for improvement that could be considered in parallel? (I'm not sure what the answer to your question is and am maybe hoping to not have to :) ) |
|
@glasser I assume that the mention of NaNs should be also removed from this section: https://spec.graphql.org/June2018/#sec-Value-Completion |
|
@fedinskiy What change do you think is relevant there? This seems consistent with that mention: that mention implies that a |
leebyron
added
the
✏️ Editorial
|
@glasser do you mind taking a look at the CLA bot? |
|
I think we need to readdress @fedinskiy's comment as well. In retrospect it seems wrong that |
Fixes graphql#778. This matches the fact that you cannot represent these values in text as FloatValue, as well as the graphql-js implementation (https://github.com/graphql/graphql-js/blob/16009cbcb0109da03f2157a868817b886801095a/src/type/scalars.js#L108-L112). This was perhaps already implied by the word "fractional", but "finite" seems to be a more standard term for "IEEE 754 floats that are not infinity or NaN".
leebyron
force-pushed
the
glasser/finite-floats
branch
from
b20c639
to
8a8f9dc
Compare
leebyron
force-pushed
the
glasser/finite-floats
branch
3 times, most recently
from
18d7924
to
0f46b62
Compare
leebyron
force-pushed
the
glasser/finite-floats
branch
from
0f46b62
to
6c7e0b6
Compare
| @@ -648,7 +648,7 @@ CompleteValue(fieldType, fields, result, variableValues): | |||
| * If {completedResult} is {null}, throw a field error. | |||
| * Return {completedResult}. | |||
| * If {result} is {null} (or another internal value similar to {null} such as | |||
| {undefined} or {NaN}), return {null}. | |||
| {undefined}), return {null}. | |||
There was a problem hiding this comment.
This change is technically a normative change, not just editorial. However I did some checking and found the reference implementation (and most others) do not adhere to short-circuiting NaN to null.
They instead treat NaN as an invalid Float and throw an error, which is exactly the behavior now accurately described above.
There was a problem hiding this comment.
GraphQL.NET throws on NaN.
|
Having trouble with the CLA but will keep iterating with LFX support (https://jira.linuxfoundation.org/plugins/servlet/theme/portal/4/SUPPORT-4646). Your updates to my text seem reasonable (I haven't verified your claims about the behavior of existing implementations with NaN but believe you!). |
|
CLA fixed. I have used better web apps. |
|
Here's the relevant lines from GraphQL.js
https://github.com/graphql/graphql-js/blob/main/src/execution/execute.js#L756-L759 Re: my inline comment above and the related #836 which clarifies that NaN should fail to coerce and throw an error, GraphQL.js Float coercion for inputs and outputs does in fact throw for any non-finite value https://github.com/graphql/graphql-js/blob/main/src/type/scalars.js#L96-L111 |
|
Thanks! |
Fixes #778.
This matches the fact that you cannot represent these values in text as
FloatValue, as well as the graphql-js
implementation (https://github.com/graphql/graphql-js/blob/16009cbcb0109da03f2157a868817b886801095a/src/type/scalars.js#L108-L112).
This was perhaps already implied by the word "fractional", but "finite" seems to
be a more standard term for "IEEE 754 floats that are not infinity or NaN".