Skip to content

Fix BigInt's external representation to string #795

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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

Fix BigInt's external representation to string #795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
"lint": "eslint --ext .ts \"./src/**/*.ts\"",
"build": "bob build --single",
"deploy:website": "cd website && yarn deploy",
"test": "jest --forceExit --no-watchman && yarn bundlesize",
"test": "jest --forceExit --no-watchman",
"posttest": "yarn bundlesize",
Copy link
Author

Choose a reason for hiding this comment

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

This is for using test command with a suite name like: yarn test BigInt

"prepare-release": "yarn build && yarn test",
"release": "yarn prepare-release && bob prepack && cd dist && npm publish",
"ci:release:canary": "node bump.js && bob prepack && npm publish dist --tag alpha --access public",
Expand Down
20 changes: 14 additions & 6 deletions src/scalars/BigInt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function patchBigInt() {
}
}

function coerceBigIntValue(value: bigint | number | string) {
function coerceBitIntValue(value: string | number | bigint): number | bigint {
if (isBigIntAvailable()) {
patchBigInt();
return BigInt(value);
Expand All @@ -32,22 +32,30 @@ function coerceBigIntValue(value: bigint | number | string) {
}
}

function parseBigIntValue(value: string): number | bigint {
return coerceBitIntValue(value);
}

function serializeBigInt(value: string | number | bigint): string {
return coerceBitIntValue(value).toString();
}

export const GraphQLBigIntConfig: GraphQLScalarTypeConfig<
number | string | bigint,
bigint | number
string | number | bigint,
string
> = /*#__PURE__*/ {
name: 'BigInt',
description:
'The `BigInt` scalar type represents non-fractional signed whole numeric values.',
serialize: coerceBigIntValue,
parseValue: coerceBigIntValue,
serialize: serializeBigInt,
parseValue: parseBigIntValue,
parseLiteral(ast) {
if (
ast.kind === Kind.INT ||
ast.kind === Kind.FLOAT ||
ast.kind === Kind.STRING
) {
return coerceBigIntValue(ast.value);
return parseBigIntValue(ast.value);
}
return null;
},
Expand Down
18 changes: 9 additions & 9 deletions tests/BigInt.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,12 @@ describe('BigInt', () => {
const { data, errors } = await graphql(schema, validQuery);
expect(errors).toEqual(undefined);
expect(data).toEqual({
a: 2n,
b: 2147483647n,
c: 2147483648n,
d: 2147483649n,
e: 439857257821346n,
f: 9007199254740993n,
a: '2',
b: '2147483647',
c: '2147483648',
d: '2147483649',
e: '439857257821346',
f: '9007199254740993',
});
});
it('4', async () => {
Expand All @@ -120,9 +120,9 @@ describe('BigInt', () => {
);
expect(errors).toEqual(undefined);
expect(data).toEqual({
a: { result: 2147483647n },
b: { result: 9007199254740991n },
d: { result: 2n },
a: { result: '2147483647' },
Copy link
Member

Choose a reason for hiding this comment

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

Why do we avoid using primitive bigint values? Adding toJSON or this package already fixes JSON serialization which is not related to graphql-js.
And we need a failing reproduction before accepting this PR.
https://github.com/ardatan/json-bigint-patch
Serializing BigInt as string might cause an unwanted coercion on JSON data transferred via HTTP.

Copy link
Author

Choose a reason for hiding this comment

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

Rather than implicitly intending to lose precision, it's better to let clients handle string correctly. Consider the case of using a long int as an id field. Even considering the JS clients, they may prefer string over numbers with lost precision as the BigInt() argument.

In the case of protobufjs, they already have the correct cast for the typeof value ==='number' and typeof value ==='string' cases.

But since they doesn't use the bigint primitive yet, it will be fallback empty ({ low: 0, high: 0, unsigned: false }) value.

(This problem is already being reproduced in the official example of graphql-mesh, and we should fix this in graphql-mesh anyway. Need a patch for graphql-scalars or protobufjs I think)

This is not graphql related and is a specific client issue, but considering that bigint hasn't been used in JavaScript widely, it's potentially more problematic than string.

Copy link
Author

Choose a reason for hiding this comment

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

Also, the patch would be avoided some cases. it makes the whole library incompatible in sandboxing environments or workers where global scope cannot be modified.

b: { result: '9007199254740991' },
d: { result: '2' },
});
});
});