tursodatabase / libsql-client-ts

TypeScript/JavaScript client API for libSQL
https://docs.turso.tech/sdk/ts/quickstart
MIT License
226 stars 32 forks source link

Update sqlite3.ts #113

Closed light-merlin-dark closed 10 months ago

light-merlin-dark commented 11 months ago

1n and 0n were causing error: RangeError: Loss of precision reading BigInt (0)

This resolved it:

else if (typeof value === "boolean") {
        return value ? 1 : 0;
    }

Was there a special intention to convert a boolean to a bigint? If not or if this was in error, it seems like this resolves and simplifies this part.

Thank you for all of your time and energy into this wonderful library.

πŸ€πŸ™πŸΌπŸŒΉβš‘οΈβœ¨

penberg commented 11 months ago

Hey @light-merlin-dark! Can you share the code snippet that was causing you the original error? I am not actually sure why we encode booleans with BigInt, but it's also part of the libSQL remote protocol ("hrana") so it's probably hard to change.

light-merlin-dark commented 11 months ago

Hey @penberg, good day! β˜€οΈ

For clarity, from the history of sqlite3.ts, it looks like the change was introduced in the commit: add more tests for numeric values.

I encountered the issue in my stack with bun and drizzle-orm. I did a push to a remote turso db and then upon reading a table I encountered the error above.

I made the change to revert it back to use normal integers and that resolved the error.

In further research with ChatGPT 4, it conveyed that the code to convert booleans as Bigints didn't seem standard unless it was for future-proofing or something like that.

All for clarity, and thanks again for the wonderful library to all who've contributed so far.

πŸ€πŸ™πŸΌπŸŒΉβš‘οΈβœ¨

giovannibenussi commented 10 months ago

Hey @light-merlin-dark, are you still having this issue? I just tested pushing a schema and then reading a table using bun and drizzle-orm and it worked well for me. Do you have a way to reproduce this issue to investigate it further?

light-merlin-dark commented 10 months ago

Hey, @giovannibenussi. Good day! β˜€οΈ

Regarding the issue, I got past it with my update to that part of the code and I currently don't have the context to revert back or check again.

It seems like something with a nature that can be determined in it's local context, whether to convert a boolean the previous way or a more common way and what support that may have across libraries. Of course, I think the most compatible and performant way is the way to go, and it seems like, with the nature of that specific function it's easy to determine if it serves it's purpose without introducing an error.

Thanks again for building such a wonderful library. I wish you and everyone here such a great day.

πŸŽ©πŸŒΉπŸŒ™πŸͺ½βœ¨

giovannibenussi commented 10 months ago

Thanks! Sounds good to me. The current code looks good to me and your issue is solved so I'll close this PR.

giovannibenussi commented 10 months ago

Opening this ticket since we found a new use case for this.

Nmans01/lib-bool-reproduce is a simple reproducer project that uses Bun and Prisma. When run it, you see the same Loss of precision reading BigInt (1) error than this issue mentions.

The cause is the boolean inside the data that is being saved:

    data: {
        name: 'Jon',
        enabled: true,
    }

To confirm this, you can do the following:

Since this is a fairly common use case, I think that we should merge this PR unless there's a strong reason to not do so.

Edit: this is being worked on this PR.