denodrivers / sqlite3

The fastest and correct SQLite3 module for Deno runtime
https://jsr.io/@db/sqlite
Apache License 2.0
265 stars 22 forks source link

feat!: add open() function #72

Closed Symbitic closed 1 year ago

Symbitic commented 1 year ago

This makes it possible to open a database later or after closing. Server applications like Fresh need this because the database path might not be known at program startup.

DjDeveloperr commented 1 year ago

This same functionality can be achieved by creating a new Database object itself only when needed. Such as

// proposed solution:
const db = new Database();
// ...
db.open("...");

// why not this?
let db: Database;
// ...
db = new Database("...");

First one would create DB with handle as null pointer which could cause some undefined behavior if somehow sent to native side. I feel like the latter is better approach. What are your views on this?

Symbitic commented 1 year ago

That might cause problems for classes (#db: Database!) or classes that extend Database. I can add a if (!#open) throw new Error("Database not open"); error to the beginning of every method. Should probably do that anyway since database can be closed.

DjDeveloperr commented 1 year ago

That might cause problems for classes (#db: Database!) or classes that extend Database.

Sorry, I think I'm unable to understand the problems here. Can you give an example? I haven't seen this API pattern before.

Symbitic commented 1 year ago
class MyClass {
  #db: Database;

  constructor() {}
}

Will cause an error because Database isn't created in the constructor. So will this:

export class MyDatabase extends Database {
  constructor() {}
}
DjDeveloperr commented 1 year ago

You can do #db!: Database in the former though

Symbitic commented 1 year ago

True, but that doesn't solve the latter. It also wouldn't solve the issue of how to reopen a closed database.

DjDeveloperr commented 1 year ago

The latter is more of an alternative for the former approach. And in case we do need to reopen a database, is there anything wrong with just creating new Database object?

Symbitic commented 1 year ago

Technically those are workarounds, but this is starting to feel like a hack to get around a basic feature. open is part of the SQLite API; why shouldn't this wrapper class have it too?

Side note: even without an open method, as long as there's a close method, every other method should add if (!this.#open) throw new Error("Database was closed"); to the beginning to prevent accessing an invalid pointer.

DjDeveloperr commented 1 year ago

sqlite3_open function in the C-API is a constructor for sqlite3* object. The wrapper for it does the same right now. I haven't seen an API like this implemented in other major database drivers before so I'm bit unsure of adding it.

Symbitic commented 1 year ago

Do you want to close this?

DjDeveloperr commented 1 year ago

This API is not a common pattern among database drivers and there isn't a strong enough use-case that is not already solved without it. Moreover it introduces a breaking change in API (open was a property which will become a method). I'll close this for now, but if it gains traction in future, it can be reopened.