Added partial BigInt support #471
Conversation
Moved from es6 to support bigint Changed from getNumber to getBigInt Avoided blobal redeclaration of function/variables Improved BigInt support, handled bind params for bigInt Logical change on how big int is supported BigInt is not fully supported at WASM level Added config to enable and disable bigInt support Added documentation Changed var to const as per readme standard
There was a problem hiding this comment.
Overall this looks good, thank you !
A few things:
- Let's stay compatible with es6, just add a dynamic check for BigInt
- We support environments that don't support the default function argument value syntax.
- remove dead code
- it looks like you implemented support for binding BigInts, but added to the README "Binding
BigIntis still not supported" - We need tests
.eslintrc.js
Outdated
| @@ -3,7 +3,7 @@ | |||
| module.exports = { | |||
| env: { | |||
| browser: true, | |||
| es6: true, | |||
| es2020: true, | |||
There was a problem hiding this comment.
I think we can avoid having to switch to es2020, and keep compatibility with environments without bigint.
You can add
if (typeof BigInt !== "function") throw new Error("BigInt not supported");before using it.
src/api.js
Outdated
| pos = this.pos; | ||
| this.pos += 1; | ||
| } | ||
| this.db.handleError(sqlite3_bind_int64(this.stmt, pos, num)); |
There was a problem hiding this comment.
Does this work ? Don't you need to convert the integer to a string for it to pass the wasm frontier ?
Also, isn't this dead code ? The method name isn't quoted so it's not usable from the outside.
There was a problem hiding this comment.
I think the best option is to remove it since BigInt is not supported at WASM level and yes I think this is a dead code since on bindValue for bigint, bindString is called.
Regarding tests, I tried writing some tests on BigInt and seems like the current test environment wasn't supporting BigInt. What might be the solution to this? |
Either add setup-node to the CI with a recent node version, and in the tests ignore the test when BigInt is not present. Or add a BigInt polyfill in the tests. |
Working on it, thanks. |
src/api.js
Outdated
| @@ -400,6 +418,11 @@ Module["onRuntimeInitialized"] = function onRuntimeInitialized() { | |||
| for (var field = 0; field < ref; field += 1) { | |||
| switch (sqlite3_column_type(this.stmt, field)) { | |||
| case SQLITE_INTEGER: | |||
| var getfunc = config && config.useBigInt | |||
There was a problem hiding this comment.
Maybe instead of config && add config = config || {} at the start of the function. This will make it easier to extend in the future.
There was a problem hiding this comment.
Maybe instead of
config &&addconfig = config || {}at the start of the function. This will make it easier to extend in the future.
I think I did miss something, I see an issue on minified files. When we pass config object { useBigInt: true } , config.useBigInt ? .... will always be evaluated as false since object property name useBigInt will be mangled to something like l.Ty. So, it will be something like l.Ty ? .... , of which Ty is not a property of l at run time. Will it be possible to have a way to prevent object properties mangling ? any idea on how to move forward on this?
There was a problem hiding this comment.
Yes, you should use config["useBigInt"]
There was a problem hiding this comment.
Maybe instead of
config &&addconfig = config || {}at the start of the function. This will make it easier to extend in the future.
Just pushed the changes, thanks
|
Your work is up on npm :) https://www.npmjs.com/package/sql.js Thanks again for your contribution, @kileha3 |
|
Much appreciated, thanks for maintaining one of the wonderful libs.
…On Thu, Aug 12, 2021, 15:59 Ophir LOJKINE ***@***.***> wrote:
Your work is up on npm :) https://www.npmjs.com/package/sql.js
Thanks again for your contribution, @kileha3 <https://github.com/kileha3>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#471 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFS47A5HQW3VBWMJB34OFD3T4PATHANCNFSM5B5Z5PIQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
BigInt is currently not fully supported at WASM level, we have added its support only when getting data from the database.