-
Notifications
You must be signed in to change notification settings - Fork 44
chore: add json-bigint and use it for SQL job parsing #481
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
base: main
Are you sure you want to change the base?
Conversation
SanjulaGanepola
left a comment
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.
@dariocs Thanks for taking a look and fixing this. Changes look mostly good. Just a couple comments and then we can get this in
| "showdown": "^2.1.0", | ||
| "sql-formatter": "^14.0.0" | ||
| "sql-formatter": "^14.0.0", | ||
| "json-bigint": "^1.0.0" |
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.
Under the devDependencies, can you also add the types: "@types/json-bigint": "^1.0.0"
src/connection/sqlJob.ts
Outdated
|
|
||
| // Use json-bigint to safely parse very large integers without losing precision. | ||
| /* eslint-disable @typescript-eslint/no-var-requires */ | ||
| const JSONbig = require('json-bigint')({ storeAsString: true }); |
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 use require, you can import at the top of the file like this:
import JSONBig from 'json-bigint';and then use:
const jsonBig = JSONBig({ storeAsString: true });
src/connection/sqlJob.ts
Outdated
| if (this.isTracingChannelData) ServerComponent.writeOutput(thisMsg); | ||
| try { | ||
| let response: ServerResponse = JSON.parse(thisMsg); | ||
| const response: ServerResponse = JSONbig.parse(thisMsg) as ServerResponse; |
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.
| const response: ServerResponse = JSONbig.parse(thisMsg) as ServerResponse; | |
| const response: ServerResponse = jsonBig.parse(thisMsg) as ServerResponse; |
Description
This commit adds json-bigint as a dependency and updates the SQL job parsing logic to use it for JSON deserialization of messages coming from the server component.
What changed
Added dependency:
package.json: added "json-bigint": "^1.0.0"
package-lock.json: updated accordingly
Parsing logic:
src/connection/sqlJob.ts: replaced the previous JSON.parse approach with json-bigint parsing to reliably handle large integers returned by the server.
Why
Some server responses include numeric values that exceed JavaScript’s Number safe integer range. Using json-bigint preserves those values (configured to store big integers as strings), avoiding precision loss and potential bugs when handling identifiers or large counters. Reference issues:
#471
Behavioral notes
Big integers in server messages are preserved as strings (no precision-losing conversion to Number).
The external API and event flow remain the same; only the JSON parsing implementation changed.
No changes to public APIs or extension activation behavior are expected.
How I tested
Confirmed the dependency declaration is present in package.json.
Installed dependencies locally and verified the project installs (note: ensure TLS/CA issues are resolved in your environment if native modules are downloaded).
Observed sqlJob message parsing and verified that large numeric fields are preserved as strings in the parsed object (manual run).
Checklist
console.logs I added