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

Feature: add Drizzle support @effect/sql-drizzle #2896

Closed
wants to merge 11 commits into from

Conversation

ecyrbe
Copy link
Contributor

@ecyrbe ecyrbe commented Jun 1, 2024

Type

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

This PR add support for drizzle in effect.
It uses internal Drizzle DB, and disable drizzle runtime execution (execute and transaction) to leverage on effect sql client

example

const SqlLive = Sqlite.client.layer({
  filename: Config.succeed("test.db")
})
const DrizzleLive = SqliteDrizzle.layer.pipe(
  Layer.provide(SqlLive)
)
const DatabaseLive = Layer.mergeAll(SqlLive, DrizzleLive)

// usage

const users = D.sqliteTable("users", {
  id: D.integer("id").primaryKey(),
  name: D.text("name")
})

Effect.gen(function*() {
  const sql = yield* Sql.client.Client
  const db = yield* SqliteDrizzle.SqliteDrizzle
  yield* sql`CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)`
  const inserted = yield* db.insert(users).values({ name: "Alice" }).returning()
  yield* Console.log(inserted)
  const selected = yield* db.select().from(users)
  yield* Console.log(selected)
  const updated = yield* db.update(users).set({ name: "Bob" }).where(gte(users.id, 1)).returning()
  yield* Console.log(updated)
  const deleted = yield* db.delete(users).where(gte(users.id, 1)).returning()
  yield* Console.log(deleted)
}).pipe(
  Effect.provide(DatabaseLive),
  Effect.runPromise
)

Related

  • Related Issue #
  • Closes #

Copy link

changeset-bot bot commented Jun 1, 2024

🦋 Changeset detected

Latest commit: 81748f0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@effect/sql-drizzle Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

let makeClient: Effect.Effect<Pg.client.PgClient, never, Scope>

beforeAll(async () => {
pgContainer = await new PostgreSqlContainer("postgres:alpine").start()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using testcontainer, not sure that's the standard way to test db in effect. tell me if it's ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testcontainer is really great, I usually wrap it in a layer so I can have a TestDb that spins up and down automatically, but this looks fine too

Comment on lines +31 to +34
options: {
parsers: [],
serializers: []
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to make drizzle patching of postgres-js client instance working. drizzle patching is not used in our case, since db instance is just used to convert query builder postgres dialect to sql

@mikearnaldi
Copy link
Member

packages/sql-drizzle/src/internal/registry.ts

Seems like the above file creates a global map Dialect<->Client but the Client is scoped and we might have many different clients in the same project.

Additionally, patching the global drizzle type would make every client effectable, even if registration didn't happen.

I think a better idea would be to yes make every drizzle query effectable but use Effect<T, SqlError, Client> so that the client is passed to the query when used

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Jun 1, 2024

packages/sql-drizzle/src/internal/registry.ts

Seems like the above file creates a global map Dialect<->Client but the Client is scoped and we might have many different clients in the same project.

This is explained in the comment of the code. We store the instance of the dialect, and a new instance of the dialect is created for each db... so each client will have a different dialect associated with it, effectively allowing to have many clients in the same project.

Additionally, patching the global drizzle type would make every client effectable, even if registration didn't happen.

I think a better idea would be to yes make every drizzle query effectable but use Effect<T, SqlError, Client> so that the client is passed to the query when used

Nice idea, it would indeed remove this global registry. i'll try it to see if it works and report back ASAP.

@mikearnaldi
Copy link
Member

packages/sql-drizzle/src/internal/registry.ts

Seems like the above file creates a global map Dialect<->Client but the Client is scoped and we might have many different clients in the same project.

This is explained in the comment of the code. We store the instance of the dialect, and a new instance of the dialect is created for each db... so each client will have a different dialect associated with it, effectively allowing to have many clients in the same project.

Additionally, patching the global drizzle type would make every client effectable, even if registration didn't happen.
I think a better idea would be to yes make every drizzle query effectable but use Effect<T, SqlError, Client> so that the client is passed to the query when used

Nice idea, it would indeed remove this global registry. i'll try it to see if it works and report back ASAP.

Ok so it isn't the dialect itself but a specific instance created with it's own client, then I think it could work, it might even be better compared to Effect<T, SqlError, Client> as it would capture the client on creation but the issue remains on having all db instances effectable regardless of the client being attached.

Also small note, packages/sql-drizzle/src/internal/drizzle-patch.types.ts should not be on /internal given that it is exposed

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Jun 1, 2024

Ok so it isn't the dialect itself but a specific instance created with it's own client, then I think it could work, it might even be better compared to Effect<T, SqlError, Client> as it would capture the client on creation but the issue remains on having all db instances effectable regardless of the client being attached.

I have a solution for that, patch SQLiteSelectBase, SQLiteInsertBase... types directly. that's what i did first. but the types are ugly :)
It's less pretty to maintain, but i can do it. My supposition was that if you use effect drizzle, you probably want to have all your instances effectable. But yeah, you may want to adopt it incrementaly.

Also small note, packages/sql-drizzle/src/internal/drizzle-patch.types.ts should not be on /internal given that it is exposed

Indeed.

/**
* QueryPromise class is used for every Drizzle QueryBuilders, it's what allows Builders to be awaitable
* So since we need all QueryBuilders to also be effectable, we patch it's interface to extend Effect
* We don't however monkey patch the QueryPromise class itself, but the QueryBuilders that extend it to be able to attach the right client to them
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patching QueryPromise might be cleaner. Is it exposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's exposed

Copy link
Contributor Author

@ecyrbe ecyrbe Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But patching QueryPromise with dependency injecting the client will patch all instances (not just for the client we declared), it's indeed doable, but @mikearnaldi has concerns about patching globally the types, so patching the propotype globally would be even more intrusive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But patching QueryPromise with dependency injecting the client will patch all instances (not just for the client we declared), it's indeed doable, but @mikearnaldi has concerns about patching globally the types, so patching the propotype globally would be even more intrusive.

I don't really have concerns if the patching is valid, the issue would be patching instances globally that can't be used in an effect environment, if the patch adds Effect<X, SqlError, Client> it is valid because the client will need to be provided so as long as augmentation and patching are done at the same time it should work

Copy link
Contributor Author

@ecyrbe ecyrbe Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i tested it, and patching QueryPromise does not work. No idea why, i get this error :

Unknown Error: TypeError: yield* (intermediate value)(intermediate value)(intermediate value)(intermediate value)(intermediate value)(intermediate value) is not iterable
 ❯ test/sqlite.test.ts:31:43
     29|       const inserted = yield* db.insert(users).values({ name: "Alice" }).returning()
     30|       assert.deepStrictEqual(inserted, [{ id: 1, name: "Alice" }])
     31|       const selected = yield* db.select().from(users)
       |                                           ^

have you ever seen it ? from what i read it's usually when detructuring undefined values.
QueryPromise patching must break something in drizzle

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another idea, will try it out and report back if it works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated #2860

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Jun 1, 2024

So, i updated the PR.
It now uses dependency injection for client instead of registry. I feel it's safer this way.
It allows to not have any dependency on private dialect property, making this implementation more stable in time
I added the type patching at Builder level instead of QueryPromise. Like i said it's ugly, and risky, since any parameter change in the builder types (renaming, adding new ones) will break typescript inference for users.

I feal that the best combination is to :

  • patch QueryBuilders without registry and use dependency injection
  • patch QueryPromise type as it only have one type parameter making it more stable

what do you think ? do i revert back to QueryPromise type Patching ?

@ecyrbe
Copy link
Contributor Author

ecyrbe commented Jun 5, 2024

closing, since tim re-took the lead on this.

@ecyrbe ecyrbe closed this Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants