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

Large Integers and GraphQLInt #292

Closed
bringking opened this issue Feb 12, 2016 · 7 comments
Closed

Large Integers and GraphQLInt #292

bringking opened this issue Feb 12, 2016 · 7 comments

Comments

@bringking
Copy link

This is related to mickhansen/graphql-sequelize#144, but I noticed in the spec that GraphQL limits integers to 2^31, and JS "Number" goes up to 2^53. We ran into errors in our application because we were using BigInt columns in the DB, and the GraphQLInt was returning null for those values. So I am wondering the "best" path forward-

  • Implement our own Scalar with GraphQLBigInt that doesn't enforce the spec limit
  • Use GraphQLFloat because in the JS implementation you use parseFloat which as a bug/feature/side-effect returns valid integers for large input integers. e.g
parseFloat("300494988383") // returns 300494988383, not 300494988383.0

Is this something that could be fixed here or is this library meant to stay strictly spec compliant?

@leebyron
Copy link
Contributor

This library is the reference implementation and designed to be spec compliant. Ints are 32-bit because many clients represent them that way. They're not 64-bit because many clients (including JavaScript) cannot represent double-wide integers.

Javascript can refer to whole numbers without losing detail up to 2^53 because JavaScript treats all numbers as floats (https://en.wikipedia.org/wiki/IEEE_floating_point). So it's definitely intentional that using GraphQLFloat will allow you to use the same numeric range JavaScript allows.

You're certainly welcome to implement a custom scalar like BigInt. We considered doing this, but determined that it was too weird to spec a 52bit integer type, as that's really only something JavaScript reasons about, and GraphQL is designed to be used across many platforms.

@leebyron
Copy link
Contributor

Just a forewarning to use caution representing BigInt (64bit) values from a database as numbers in JavaScript. Any integer-like number above 52bit will have it's lower values truncated. I'd recommend serializing them as strings and using them in JavaScript as strings, or using something custom like https://www.npmjs.com/package/big-integer

@bringking
Copy link
Author

@leebyron Thanks again for the excellent feedback. I will suggest in the graphql-sequelize that we infer BigInt columns as strings, and the clients can choose to use something like big-integer.

@brainjam
Copy link

I'm very much a beginner in both GraphQL and Sequelize, but I hit this problem today. It looks like BIGINT is already handled as strings in Sequelize, and it is best to define its GraphQL counterparts as GraphQLString (and then use whatever you need, such as big-integer, starting from the string).

@msakrejda
Copy link

We just ran into this as well and wasted a lot of time trying to figure out the behavior. The api reference doesn't mention a size limit on ints, but it looks like you're right: the spec does. However, that part of the spec also says

GraphQL servers should coerce non‐int raw values to Int when possible otherwise they must raise a field error. Examples of this may include returning 1 for the floating‐point number 1.0, or 2 for the string "2".

The details of coercion are left to implementers, but coercing values outside of the 32-bit int range to null seems against the spirit of the spec. Why does graphql-js not raise an error when such a value is provided? Is this behavior generally useful? We would much rather get a clear error in this situation--it seems like this is a major assert of having the graphql type system in the first place.

Thoughts?

@msakrejda
Copy link

I guess maybe this is not getting attention because the issue is closed. I'll file a new issue.

@Kostanos
Copy link

Kostanos commented Jul 5, 2019

If you don't want to implement BigInt support (which is native since node 10.4)
At least allow to create custom type like in this issue: #2017

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

5 participants