search

mysqljs / sqlstring

Simple SQL escape and format for MySQL
MIT License
403 stars 78 forks source link

Add support for native BigInt #49

Open glen-84 opened 4 years ago

glen-84 commented 4 years ago

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

case 'bigint': return val + '';

Is this the only required change?

glen-84 commented 4 years ago

@dougwilson ?

dougwilson commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

glen-84 commented 4 years ago

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 commented 4 years ago

@dougwilson,

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

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