kysely-org / kysely

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

[Discussion][Effect integration] Add Executable inheritence to all Executable Builders #1055

Closed ecyrbe closed 1 week ago

ecyrbe commented 1 week ago

Regarding Kysely integration with Effect, i got feedback that leveraging on Patching Kysely Builders could make Effect integration somewhat unstable to future Kysely evolutions.

Indeed, new builders could be added in the future, or removed/renamed in a breaking change. To minimalise the maintenance effort to keep Effect and Kysely in sync, i propose a little evolution.

Integration example

 const selected = yield* db.selectFrom("users").selectAll()
//           ^? const selected: { id: number; name: string; }[]

What is it doing

SelectAll is returning a SelectQueryBuilder. So in the current PR : https://github.com/Effect-TS/effect/pull/3017 I patched SelectQueryBuilder prototype and type to also be an Effect type. This way, yield* knows a query builder is also an effect and when the builder is yielded, it calls execute() method and encapsulate the resulting promise into an Effect object.

Patching the builders

This approach has the downside that in the PR i need to patch all Kysely builders that have an execute() and compile() method. So like i said, this approach is not totaly future proof.

Proposal

Something that could be done is to add and export a base class Executable so that only this class prototype and type would be patched by Effect library. This way, kysely adding/renaming/removing builders would have no impact on effect integration. No need to keep code in sync. One version of Effect would support all future Kysely versions even accross many breaking changes.

class Executable<O = unknown> {
  abstract execute(): Promise<O>
}

Now here what a it would look like for SelectQueryBuilder

export interface SelectQueryBuilder<DB, TB extends keyof DB, O> extends WhereInterface<DB, TB>, 
  HavingInterface<DB, TB>, 
  SelectQueryBuilderExpression<O>, 
  Compilable<O>, 
  Explainable, 
  Streamable<O>, 
  Executable<Simplify<O>[]> {
...
}

Would such addition be welcomed to kysely code base ? If so i can also provide a PR for it.

koskimas commented 1 week ago

No thanks. Even if you implement it, we'll need to maintain it.

ecyrbe commented 1 week ago

I understand, thanks for taking the time to read.

datner commented 1 week ago

Would it be possible to at least place this into discussion? I don't think that an integration hook should take too-big of a maintenance effort and if it then maybe we can find a less demanding solution?