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.

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

Jump to bottom

treat bigint as string in msnodesqlv8 driver #1387

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

treat bigint as string in msnodesqlv8 driver #1387

wants to merge 1 commit into from

Conversation

dhensby
Copy link
Collaborator

@dhensby dhensby commented Apr 19, 2022

What this does:

BigInts should be treated as strings to ensure no precision loss

Related issues:

Attempt to fix #1385

Pre/Post merge checklist:

  • Update change log

@dhensby dhensby force-pushed the pulls/bigint-msnodesql branch 2 times, most recently from 1fb3a94 to 9f21a3e Compare April 19, 2022 14:53
This was referenced Apr 19, 2022
Closed
Closed
@dhensby
Copy link
Collaborator Author

dhensby commented Apr 19, 2022

Well, interestingly, the tedious driver does not allow strings to be used for bigints and instead they must be set to varchars...

@blitzmann
Copy link
Contributor

Would there be an argument for using JS's BitInt type instead of a string? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

mssql doesn't support Node versions < 10 according to changelog, and it's been around since 10.4

@dhensby
Copy link
Collaborator Author

dhensby commented Apr 21, 2022 •
edited
Loading

Yep, definitely, but it's more about the underlying drivers than about this library. If they support either strings and/or BigInt, then we can use them.

@blitzmann
Copy link
Contributor

Ah that makes sense

@dhensby dhensby force-pushed the pulls/bigint-msnodesql branch 3 times, most recently from a7527cd to 5c27016 Compare April 28, 2022 21:07
@dhensby dhensby force-pushed the pulls/bigint-msnodesql branch from 5c27016 to e5b3f60 Compare August 19, 2022 08:52
@dhensby dhensby force-pushed the pulls/bigint-msnodesql branch from e5b3f60 to bd791d4 Compare September 4, 2023 22:26
@dhensby
Copy link
Collaborator Author

dhensby commented Sep 4, 2023

Well, interestingly, the tedious driver does not allow strings to be used for bigints and instead they must be set to varchars...

So this is the problem we have getting this merged, the tedious driver is the blocker

@dhensby
fix: treat bigint as string in msnodesqlv8 driver
a8723fb 
BigInt types will lose precision if they are above the MAX_SAFE_INTEGER
value. A work around for this is to use strings to represent the numbers
instead.
@dhensby dhensby force-pushed the pulls/bigint-msnodesql branch from bd791d4 to a8723fb Compare September 14, 2023 21:10
@dhensby
Copy link
Collaborator Author

dhensby commented Jun 18, 2024

So tedious introduced using native bigints for the bigint type in v18.2.0, which will be added in the next major release of this lib (imminent)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BigInt incorrectly cast to int with msnodesqlv8
2 participants
@dhensby @blitzmann