Skip to content

BigInt columns should be GraphQLFloat #144

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

Closed
bringking opened this issue Feb 4, 2016 · 5 comments
Closed

BigInt columns should be GraphQLFloat #144

bringking opened this issue Feb 4, 2016 · 5 comments

Comments

@bringking
Copy link
Contributor

Wanted to get your thoughts on this issue we just ran into.

The graphql spec limits GraphQLInt values to 2^31.

 const num = parseInt(ast.value, 10);
  if (num <= MAX_INT && num >= MIN_INT) {
    return num;
  }

However, we are converting BigInt columns to GraphQLInt, which silently returns those over 2^31 as null. Changing this type mapping to GraphQLFloat (which feels slightly wrong, since they are not floating point numbers) allows the numbers to be not-null (at least up to the JS limit of 2^53. Thoughts?

@mickhansen
Copy link
Owner

No firm opinion, any clues in the spec?

@bringking
Copy link
Contributor Author

Well the spec specifies that a GraphQLFloat is definitely a FP number-

A Float number includes either a decimal point (ex. 1.0) or an exponent (ex. 1e50) or both (ex. 6.0221413e23).

However, the graphql-js implementation just uses parseFloat

parseFloat(ast.value)

which in our case, passing a large integer value to parseFloat, results in standard integer, e.g.

parseFloat("4121722835") // result: 4121722835
// not
// 4121722835.0

So in order to get a BigInt, it seems we have to use either a custom scalar type or use the Float type because of this behavior. I guess the "cleanest" way to do this would be to implement our own Scalar type called BigInt that doesn't adhere to the GraphQLInt limits. Then the question becomes whether this library starts implementing custom types. In this case, I am leaning towards the custom type as being the best solution...

@mickhansen
Copy link
Owner

A custom scalar is probably the best way to go if there's no standard way of doing int8

@brainjam
Copy link

The natural mapping would be to GraphQLString wouldn't it? If you need to further play with it on the GraphQL side you can convert to big-integer or float or whatever. My use case is that the BIGINT is a 64-bit integer unique iD, so I'm happy enough with the string representation on the GraphQL side.

@bringking
Copy link
Contributor Author

Yeah I agree with @brainjam after talking it over with Lee byron on the other projects. I will do a PR making GraphQLString the default.

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

3 participants