-
Notifications
You must be signed in to change notification settings - Fork 138
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
Fix BigInt's external representation to string #795
Conversation
@@ -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", |
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.
This is for using test command with a suite name like: yarn test BigInt
Let me know what you think about my comments @cometkim |
@ardatan I think I missed it. what comments do you mean? |
a: { result: 2147483647n }, | ||
b: { result: 9007199254740991n }, | ||
d: { result: 2n }, | ||
a: { result: '2147483647' }, |
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.
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.
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.
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.
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.
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.
Closed in favor of ardatan/graphql-mesh#1987 and protobufjs/protobuf.js#1592 |
Description
This kinda breaking change, fixed the BigInt's external representation to vanilla string
Related ardatan/graphql-mesh#1355
Since BigInt objects can't be serialized to JSON, we're monkey-patching
BigInt.prototype.toJSON
. but it seems like a different path is actually being called from graphql-mesh.So I think we should change the internal and external representation to handle it more explicitly to resolve the graphql-mesh issue and further potential issues.
Type of change
How Has This Been Tested?
ardatan/graphql-mesh#1355 (comment)
Test Environment:
Checklist: