sqlstring icon indicating copy to clipboard operation
sqlstring copied to clipboard

Add support for native BigInt

Open glen-84 opened this issue 6 years ago • 6 comments

https://github.com/mysqljs/sqlstring/blob/8f193cae10a2208010102fd50f0b61e869e14dcb/lib/SqlString.js#L39

case 'bigint': return val + '';

Is this the only required change?

glen-84 avatar Dec 13 '19 19:12 glen-84

@dougwilson ?

glen-84 avatar Jan 07 '20 13:01 glen-84

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

dougwilson avatar Jan 07 '20 13:01 dougwilson

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.)

glen-84 avatar Jan 07 '20 14:01 glen-84

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 avatar Jan 07 '20 14:01 dougwilson

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 avatar Jan 07 '20 17:01 glen-84

@dougwilson,

Any updates on this or https://github.com/mysqljs/mysql/issues/2306?

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

glen-84 avatar Jul 31 '20 08:07 glen-84