kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.44k stars 266 forks source link

SqliteStatement interface should allow undefined #769

Closed mnpenner closed 10 months ago

mnpenner commented 10 months ago

https://github.com/kysely-org/kysely/blob/23655565f32fdab79190eaa550227c35e4da03d6/src/dialect/sqlite/sqlite-dialect-config.ts#L41-L42

It looks like Kysley handles null and undefined perfectly fine,

https://github.com/kysely-org/kysely/blob/23655565f32fdab79190eaa550227c35e4da03d6/src/dialect/sqlite/sqlite-driver.ts#L79-L89

And if we're not using autoincrement, then lastInsertRowid really should be null or undefined, no?

koskimas commented 10 months ago

What's the issue here? What code are you running, and how is it failing?

The internal types you're referring to are copied from the driver library and shouldn't affect the userland code in any way.

mnpenner commented 10 months ago

I want to use Kysely with Bun+Sqlite, but the docs say that's unsupported:

image

So I wrote a wrapper:

import {SqliteDatabase, SqliteStatement} from 'kysely'
import Database, {Statement} from 'bun:sqlite'

export class BunSqliteDatabase implements SqliteDatabase {
    private db: Database

    constructor(...args: ConstructorParameters<typeof Database>) {
        this.db = new Database(...args)
    }

    close(): void {
        this.db.close()
    }

    prepare(sql: string): SqliteStatement {
        return new BunSqliteStatement(this.db, sql)
    }
}

type RunResult = { changes: number | bigint; lastInsertRowid: number | bigint }

class BunSqliteStatement implements SqliteStatement {
    readonly reader: boolean
    private stmt: Statement

    constructor(private db: Database, sql: string) {
        this.stmt = this.db.prepare(sql)
        this.reader = /^\W*SELECT\b/i.test(sql)
    }

    all(parameters: ReadonlyArray<unknown>): unknown[] {
        return this.stmt.all(...parameters)
    }

    run(parameters: ReadonlyArray<unknown>): RunResult {
        this.stmt.run(...parameters)
        return this.db.prepare<RunResult,[]>("SELECT changes() as changes, last_insert_rowid() as lastInsertRowid").get()!
    }
}

Which I can use like this:

export const db = new Kysely<DB>({
    dialect: new SqliteDialect({
        database: new BunSqliteDatabase(process.env.DATABASE_URL)
    })
})

It seems to work perfectly fine, but again, the typings are not right. last_insert_rowid() might be null. Or maybe 0, I'm not sure what it does when the table isn't auto-increment. Originally I just returned {changes: undefined, lastInsertRowid: undefined} but then TS really complains.

The return value for SqliteStatement.run isn't what I'd call internal because the interface is exposed.

koskimas commented 10 months ago

As I said, those types are copied from the official better-sqlite3 typings. The kysely driver is just defensive and checks for null and undefined just in case. Based on the types, those can never be null or undefined.

If you an provide proof that better-sqlite3 does indeed return null or undefined in those sometimes, I'll change the types. But in that case you should open an issue in the official better-sqlite3 typing repo as well.

mnpenner commented 10 months ago

I'm not using better-sqlite3 though. Just the {SqliteDatabase, SqliteStatement} interfaces to make a Bun's implementation compatible to avoid having to implement a full driver. But if it makes it harder to update the definitions later I guess it's fine to leave it as-is, not that big of a deal.

koskimas commented 10 months ago

You probably shouldn't try to fit the better-sqlite types to some other library.