Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support any database via hook / callback pattern #6

Closed
wants to merge 1 commit into from

Conversation

jakobo
Copy link

@jakobo jakobo commented Nov 14, 2024

Motivation

This enables developers to "swap" in any database layer should they outgrow the default sqlite implementation. This also makes it possible to bring in sqlite alternatives like Turso which rely on network connections and therefore must be asynchronous.

The hooks/callback pattern remains a successful DB adapter pattern from Auth0, Clerk, and others, which do not dictate an ORM layer, leaving that to the person who is extending the core functionality.

Summary of Changes

  • Makes all methods asynchronous, minimizing the total changeset. I realize this can introduce unhandled exceptions if the async methods threw, but I wanted to prioritize a smaller diff
  • Adds a DBCallbacks type that describes the current set of database callbacks
  • Moves the sqlite operations into their own set of callbacks in src/util/sqlite.ts for backwards compatibility
  • Updates the readme, referencing the sqlite code as a template other developers can reference

Risks

This exposes a new API which comes with backwards compatibility concerns. Adding new functionality to the labeler which also adds a new Database call becomes a breaking change. I don't think this is a concern, as people should be pinning their dependency on skyware.

Open Issues

  • getAllLabels in the driver seems to be missing a limit and I can't for the life of me remember if that's automatically handled by stmt.iterate(cursor) or not.
  • Would we want DBCallbacks to be part of the exports

@futurGH
Copy link
Contributor

futurGH commented Nov 18, 2024

Thanks for the PR! After quite a bit of thought, I'm going to be closing this in favour of #4.

I don't think "outgrowing" SQLite is a concern right now; some of the largest labelers with a 6-7 digit label count are currently running without issue off an SQLite database.

The other issue is that this would make it impossible to expose the default SQLite database for directly querying in a type-safe way without requiring projects to import and initialize libsql themselves. This is functionality that many existing labelers depend on, which I'm hesitant to change out from under them.

TL;DR: @libsql/client accounts for remote databases, I don't foresee performance being an issue anytime soon, and this would significantly negatively impact existing usage patterns.

@futurGH futurGH closed this Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants