-
Notifications
You must be signed in to change notification settings - Fork 9
Add spec for Long custom scalar type
#26
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
66fb671
086bfff
efeab84
e20ed45
28ba686
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,79 @@ | ||||||||
| <!-- cspell:ignore <github user name> --> | ||||||||
|
|
||||||||
| # Long — GraphQL Custom Scalar | ||||||||
|
|
||||||||
| Author - jakobmerrild | ||||||||
|
|
||||||||
| Date - 2024-12-16 | ||||||||
|
|
||||||||
| **License and Copyright** | ||||||||
|
|
||||||||
| Copyright © GraphQL contributors. This specification is licensed under | ||||||||
| [OWFa 1.0](https://www.openwebfoundation.org/the-agreements/the-owf-1-0-agreements-granted-claims/owfa-1-0). | ||||||||
|
|
||||||||
| # Overview | ||||||||
|
|
||||||||
| This Scalar represents a 64-bit signed integer (non-fractional) value, ranging from `-2^63` to `2^63-1`. Such values are commonly | ||||||||
| used to represent timestamps as the number of milliseconds since the UNIX epoch (`1970-01-01T00:00:00.000Z`) | ||||||||
| among other use cases. | ||||||||
|
|
||||||||
| # Name | ||||||||
|
|
||||||||
| The scalar should be named `Long` to match commonly used names for the same data structure in a variety of programming languages. | ||||||||
| Alternatively the scalar can be named `Int64` to represent the 64-bit encoding. | ||||||||
|
|
||||||||
| # Result JSON spec | ||||||||
jakobmerrild marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
|
|
||||||||
| A field of type `Long` should result in a JSON `number` without a fractional or exponential part. A leading `-` should only be | ||||||||
| added if the field represents a negative value. | ||||||||
martinbonnin marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
|
|
||||||||
| These are valid examples: | ||||||||
|
|
||||||||
| | Output | Explanation | | ||||||||
| | ---------------------- | -------------------------------------------------------------- | | ||||||||
| | `0` | Zero is a valid integer within the range | | ||||||||
| | `-9223372036854775808` | This is the lowest value that can be represented in the range | | ||||||||
| | `9223372036854775807` | This is the largest value that can be represented in the range | | ||||||||
|
|
||||||||
| These are invalid examples: | ||||||||
|
|
||||||||
| | Output | Explanation | | ||||||||
| | ----------------------- | ------------------------------------------------------------------------ | | ||||||||
| | `+1234` | Leading `+` is not allowed | | ||||||||
| | `-10223372036854775808` | Value is lower than `-2^63` | | ||||||||
| | `12223372036854775807` | Value is greater than `2^63-1` | | ||||||||
| | `123.0` | Fractional part is not allowed even if it is zero | | ||||||||
| | `1e6` | Exponential notation is not allowed, even if it represents a valid value | | ||||||||
| | `"12345"` | String representations of a valid value are not allowed | | ||||||||
|
|
||||||||
| # Literal Input spec | ||||||||
|
|
||||||||
| For input both IntValue and StringValue shall be accepted so long as the StringValue is a base-10 representation of a | ||||||||
| valid value within the range. | ||||||||
|
||||||||
| For input both IntValue and StringValue shall be accepted so long as the StringValue is a base-10 representation of a | |
| valid value within the range. | |
| Inputs should be a `number` between `-2^63` and `2^63-1`. That number must not have a fractional or exponential part. A leading `-` is added to represent negative values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really on the fence w.r.t. String. On the one hand, Javascript's number doesn't accurately represent the full range of the spec, and Javascript is one of, if not the most, used language for GraphQL. On the other hand it seems weird to allow string values, especially if they aren't allowed in both the input and output. If string values are not allowed, then all Javascript implementations of the spec will have to have special handling of the incoming JSON, assuming JSON is used for (de)serialization, to avoid issues where JSON.parse incorrectly parses the number.
E.g. https://codesandbox.io/p/devbox/lr8z8x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that Javascript now supports BigInt, but I don't know how widely adopted it is. And either way, JSON.parse doesn't parse a JSON number into BigInt (how could it, number can have fractional parts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is clear that Long needs to support string values because JSON numbers are not guaranteed to support the range required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andimarek JSON supports numbers with arbitrary precision:
JSON can contain number literals of arbitrary precision. However, it is not possible to represent all
JSON numbers exactly in JavaScript, because JavaScript uses floating point representation which
has a fixed precision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oerall, I think JavaScript is mostly irrelevant here? Input values are either:
- GraphQL IntValue litteral (parameter values in executable documents): supports arbitrary precision
- JSON number litteral (json variables in the request): supports arbitrary precision
Is the concern about programmatically provided values?
I would leave this out of the scalar specs and up to the implementations. This is not something that goes over the wire and I'm fine leaving this unspecified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I am reading over the guidelines again I notice the following:
Additionally, the input coercion should be liberal in what it accepts, while the result coercion should be much more restricted and never produce different JSON values for logically identical values. For example a MyLocalDate scalar could accept the literals "01-10-2022" and "01102022" as input for the first of October 2022, but the result coercion should always return one of the possible representations.
Given these are still the guidelines I propose we keep the spec such that it allows string or number for input coercion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with the guideline and will make a separate PR to open that discussion (edit: here). The robustness principle was a thing in the eighties but these days, it's generally considered harmful, there's even a RFC about it.
Ultimately, this is your place and we don't have to agree. Feel free to resolve this discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the spec is better off just allowing numbers. However, I was trying to figure out why I decided to include string as acceptable input originally. I believe the following factors were part of my original decision
- The current guide lines as quoted above
- The fact that Javascript doesn't play nicely with the entire range out of the box when deserializing JSON
- Related to the above, and purely selfish, we currently use
Stringto represent this data type in our own custom Scalar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjie convinced me in that comment that we need strings and we can't completely ignore JS.
JSON number is better from a wire point of view but JSON.parse() is going to lose precision and it's not like we can change that or ask users to embed their own JSON parsers. (We could but I doubt it will be well received...)
I'm now team "let's require strings in both directions", same as what @benjie is suggesting in his comment I think.
Uh oh!
There was an error while loading. Please reload this page.