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

Add support for native BigInt #49

Open
glen-84 opened this issue Dec 13, 2019 · 6 comments
Open

Add support for native BigInt #49

glen-84 opened this issue Dec 13, 2019 · 6 comments
Assignees

Comments

@glen-84
Copy link

glen-84 commented Dec 13, 2019

switch (typeof val) {

case 'bigint': return val + '';

Is this the only required change?

@glen-84
Copy link
Author

glen-84 commented Jan 7, 2020

@dougwilson ?

@dougwilson
Copy link
Member

I haven't used the native bigints yet, so cannot say off hand yes or no. I would need to check it out.

@glen-84
Copy link
Author

glen-84 commented Jan 7, 2020

I tried it a while ago and it seemed to work fine.

How would you handle writing the test when versions of Node.js prior to 10.4.0 do not support BigInt syntax/literals?

'bigints convert to strings': function() {
  // This will likely fail in Node.js versions prior to 10.4.0.
  assert.equal(SqlString.escape(9007199254740991n), '9007199254740991');
}

(BTW, is it really necessary to support Node.js all the way back to v0.6? Versions below v10 are no longer maintained.)

@dougwilson
Copy link
Member

I tried it a while ago and it seemed to work fine.

Ok. Sorry if I misunderstood your request then. I thought you were asking me to help determine if it would work or not.

How would you handle writing the test when versions of Node.js prior to 10.4.0 do not support BigInt syntax/literals?

I will need to investigate.

BTW, is it really necessary to support Node.js all the way back to v0.6? Versions below v10 are no longer maintained.

Yes, as I have use-cases for those versions. If those versions were dropped, I would no longer be able to use my own module, which seems counter-productive.

@dougwilson dougwilson self-assigned this Jan 7, 2020
@glen-84
Copy link
Author

glen-84 commented Jan 7, 2020

This should make queries work, but the result data will still return strings.

The mysql package likely requires updates to these locations (at least):

/lib/protocol/Parser.js#L203
/lib/protocol/packets/RowDataPacket.js#L99

It might also make sense to have an enableNativeBigInt option (defaulting to false) for BC, so that behaviour doesn't suddenly change when a developer switches to a version of Node.js that supports BigInt. The default could change to true in a later (major) version.

I tried to open an issue in that repository, but I don't have permission.

@glen-84
Copy link
Author

glen-84 commented Jul 31, 2020

@dougwilson,

Any updates on this or mysqljs/mysql#2306?

Can we just skip the test on older Node.js versions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants