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

BigInt support in scalars #4276

Closed
wants to merge 1 commit into from
Closed

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Nov 1, 2024

Supersedes #4088
Resolves #3913

This adds support for BigInt values when we serialize a response value from JS to send over the wire.

  • When the expected value is a String we'll stringify the BigInt
  • When the expected value is an ID we'll stringify the BigInt
  • When the expected value is an int we'll cast it to a number, when this exceeds or is below min/max int we'll still throw an error
  • When the expected value is a float we'll cast it to a number
  • When the expected value is a boolean we'll cast it to a boolean value, i.e. 1 true 0 false

For Input Values:

  • When the expected value is an ID we'll stringify the BigInt
  • When the expected value is an int we'll cast it to a number, when this exceeds or is below min/max int we'll still throw an error
  • When the expected value is a float we'll cast it to a number

This also adds support in valueToAST even though it's deprecated because BigInt support has always been in the spec when it comes to the ID Scalar.

I've tried adding BigInt to valueToLiteral however, for that we'd need to expand the spec to i.e. have an identifier for BigInt as currently we don't really have a way to represent it so we have to treat it like it is a regular number.

@JoviDeCroock JoviDeCroock requested a review from a team as a code owner November 1, 2024 06:53
Copy link

netlify bot commented Nov 1, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 54f77df
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/67247afc79b783000846f55f
😎 Deploy Preview https://deploy-preview-4276--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Nov 1, 2024

Hi @JoviDeCroock, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@JoviDeCroock JoviDeCroock added the PR: feature 🚀 requires increase of "minor" version number label Nov 1, 2024
Copy link
Contributor

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to get this in. Some comments inline are with respect to how to handle bigint => float

In terms of valueToLiteral(), I think it can be upgrade to handle bigint in the same way as astFromValue. valueToLiteral is a typed function that ends up calling the individual scalar .valueToLiteral() methods just like astFromValue() and so when supplied a bigint someBigInt like so: valueToLiteral(someBigInt, GraphQLInt), it would call GraphQLInt.valueToLiteral() and the idea would be that this should work as long as the bigint can be coerced.

This would not be about adding a bigint literal type.

src/type/scalars.ts Show resolved Hide resolved
src/type/scalars.ts Show resolved Hide resolved
@@ -280,6 +283,7 @@ describe('Type System: Specified scalar types', () => {
expect(coerceOutputValue('-1.1')).to.equal(-1.1);
expect(coerceOutputValue(false)).to.equal(0.0);
expect(coerceOutputValue(true)).to.equal(1.0);
expect(coerceOutputValue(1n)).to.equal(1n);
Copy link
Contributor

@yaacovCR yaacovCR Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expect(coerceOutputValue(1n)).to.equal(1n);
expect(coerceOutputValue(1n)).to.equal(1);

Assuming we take the change with respect to floats.

@@ -143,6 +168,11 @@ describe('astFromValue', () => {
kind: 'NullValue',
});

expect(astFromValue(9007199254740993n, GraphQLString)).to.deep.equal({
kind: 'StringValue',
value: '9007199254740993',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
value: '9007199254740993',
value: '9007199254740992',

There is a loss of precision when converting a bigint to a float

@yaacovCR
Copy link
Contributor

yaacovCR commented Nov 4, 2024

This also adds support in valueToAST

I think typo => astFromValue

@yaacovCR
Copy link
Contributor

yaacovCR commented Nov 4, 2024

Taking a step back:

  1. upgrading coerceOutputValue/serialize to handle bigint seems like a sometimes win, as resolvers can now return bigints and we convert automatically to GraphQLString or GraphQLID => but also a sometimes loss, as converting bigint to GraphQLInt or GraphQLFloat might be an error/lose precision. Even if we can coerce in an individual scenario, it suggests an underlying developer error, because we can't always coerce. It seems like forcing the developer to coerce might be the safest option.

  2. upgrading coerceInputValue/parse to handle bigint is not as big of a win from the get-go, as the natural path of value flow into GraphQL is via JSON, and JSON does not support bigint. We also in general are stricter on coercion for input => For example, in output we would convert a boolean to GraphQLInt and a number to GraphQLBoolean, but we do not do that for input types. In short, I am not sure if we should upgrade coerceInputValue/parse to handle bigint .

  3. upgrading astFromValue or valueToLiteral to support bigint means allows us to print programmatically set default values that were set using bigint. In v16 and earlier, the default values when supplied programatically are the internal values, and so we call serialize to print them, even though that may not be defined on input values. In v17, we call valueToLiteral to print them, which is well defined. In v16, that means that once we upgrade serialize, essentially astFromValue has already been upgraded. In v17, there is less to little need to upgrade valueToLiteral because the schema author should be able to supply the correct type instead of bigint when specifying the default value.

Just some longer thoughts, meant to rehearse my understanding of Lee's diagram from the default value PR.

@JoviDeCroock JoviDeCroock deleted the bigint-support-revised branch November 5, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants