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

Typings don't allow custom query builders #319

Closed
elhigu opened this issue Mar 6, 2017 · 71 comments
Closed

Typings don't allow custom query builders #319

elhigu opened this issue Mar 6, 2017 · 71 comments

Comments

@elhigu
Copy link
Contributor

elhigu commented Mar 6, 2017

I'm trying to create Model, which has extended query builder with .session() method. However when I'm extending the model and setting our custom query builder there return type of $query() and other functions still is QueryBuilder<this> instead of CustomQueryBuilder<this>.

Simplified (not runnable example):

class CustomQueryBuilder<T> extends QueryBuilder<T> {
  session(session: any) {
    this.context().session = session;
    return this;
  }
}

class BaseModel extends Model {
  // Override the objection.js query builders classes for subclasses.
  static QueryBuilder = CustomQueryBuilder;
  static RelatedQueryBuilder = CustomQueryBuilder;

  // ... more stuff ...
}

const daa = await BaseModel
          .query(trx)
          // ERROR: [ts] Property 'session' does not exist on type 'QueryBuilder<BaseModel>'
          .session(req.session)
          .insert({1:1});

Any ideas how to override this? I wouldn't like to do casting everywhere I'm using my extended query builder, nor I would like to add extra methods to BaseModel which would apply correct typing for returned query builder.

@koskimas
Copy link
Collaborator

@mceachen Could you take a look at this?

@mceachen
Copy link
Collaborator

mceachen commented Mar 13, 2017

Sure.

So, I'm pretty sure in order to expose an extension of a QueryBuilder in your model class, we'll need to extend Model with a generic type, something like:

export class Model<QB extends QueryBuilder>

The problem is that the QueryBuilder signature already has a generic typing associated to it, that points back to the defining Model, and I can't use a this keyword in the generic definition, like this:

export class Model<QB extends QueryBuilder<this>>

because

[ts] A 'this' type is available only in a non-static member of a class or interface.

I'm trying to think of a clever workaround here, but if anyone comes up with a solution, I'm receptive.

@koskimas
Copy link
Collaborator

koskimas commented Mar 19, 2017

@elhigu Can't you just do this:

class BaseModel extends Model {
  static query(...args): CustomQueryBuilder {
    return super.query(...args) as CustomQueryBuilder;
  }

  $query(...args): CustomQueryBuilder {
    return super.$query(...args) as CustomQueryBuilder;
  }

  $relatedQuery(...args): CustomQueryBuilder {
    return super.$relatedQuery(...args) as CustomQueryBuilder;
  }
}

this example won't work but you see what I mean? Override the relevant BaseModel methods to return the correct inherited type.

@elhigu
Copy link
Contributor Author

elhigu commented Mar 20, 2017

@koskimas I tried that, but typescript tells me that those methods doesn't have correct signature :(

But this syntax I learned recently, might work also for class methods I haven't tried it yet (https://www.typescriptlang.org/docs/handbook/functions.html):

function pickCard(x: {suit: string; card: number; }[]): number;
function pickCard(x: number): {suit: string; card: number; };
function pickCard(x): any {
    // Check to see if we're working with an object/array
    // if so, they gave us the deck and we'll pick the card
    if (typeof x == "object") {
        let pickedCard = Math.floor(Math.random() * x.length);
        return pickedCard;
    }
    // Otherwise just let them pick the card
    else if (typeof x == "number") {
        let pickedSuit = Math.floor(x / 13);
        return { suit: suits[pickedSuit], card: x % 13 };
    }
}

@tobalsgithub
Copy link

@elhigu I was able to get the static query function definition to work. Try changing the example given by @koskimas to

class BaseModel extends Model {
  static query(trx? : Transaction): CustomQueryBuilder {
    return super.query.call(this, trx) as CustomQueryBuilder;
  }
}

Haven't played with it too much, but the typescript does compile at least.

Haven't figured out the $query or $relatedQuery functions yet, as those seem to require a different type of QueryBuilderSingle<this>

@mceachen
Copy link
Collaborator

@tobalsgithub's approach should work, but it'd be nice if the typings "just worked" without that.

Also, be aware that you'll only have your new methods available from chained method calls that return a this (which is about half the methods). Other methods will have reverted to the default query builders.

For example, BaseModel.query().relate() will still return a CustomQueryBuilder, but BaseModel.query().insertAndFetch() will give you a QueryBuilderSingle<BaseModel>.

The only way around this that I see is introducing both generic subtypes for Model as well as generic subtypes for QueryBuilder and that hierarchical tree.

@elhigu
Copy link
Contributor Author

elhigu commented Mar 20, 2017

Hi, thanks everyone. I'll report back after I got a change to try out these solutions.

IMO if I get any of these working it is good enough workaround. Anyways at least for now this is quite rare use case and overriding $query etc. shouldn't be a show stopper for anyone..

@elhigu
Copy link
Contributor Author

elhigu commented Aug 17, 2017

@tobalsgithub I still cannot get this stuff working, but made some progress and static method overriding seem to work already. $query is still failing, because class is extended from incompatible promise stuff or something like that. I made test project and when its working I'll add it to greenkeeper (or merge to objection codebase) to keep example working when typings are updated (https://github.com/elhigu/objection-overriding-querybuilder-typescript).

import { Model, QueryBuilder, QueryBuilderSingle, Transaction } from 'objection';

class BaseQueryBuilder<T> extends QueryBuilder<T> {
  session(session: any) {
    return this.mergeContext({ session });
  }
}

// try to override $query() method to return correct query builder
class BaseModelDollarQueryOverrideTest extends Model {
  static QueryBuilder = BaseQueryBuilder;
  static RelatedQueryBuilder = BaseQueryBuilder;

  $query(trx?: Transaction): BaseQueryBuilder<this> {
    return <any> this.$query(trx);
  }
}

// try to override static query() method to return correct query builder
class BaseModelStaticQueryOverrideTest extends Model {
  static QueryBuilder = BaseQueryBuilder;
  static RelatedQueryBuilder = BaseQueryBuilder;

  static query<T>(trx?: Transaction): BaseQueryBuilder<T> {
    return <any> super.query(trx);
  }
}

const testDollar = new BaseModelDollarQueryOverrideTest();
testDollar.$query().session({});

BaseModelStaticQueryOverrideTest.query().session({});

Current error is:

Mikaels-MacBook-Pro-2:objection-subclassing mikaelle$ npm test

> objection-subclassing@1.0.0 test /Users/mikaelle/Projects/Vincit/objection-subclassing
> tsc

test.ts(10,7): error TS2415: Class 'BaseModelDollarQueryOverrideTest' incorrectly extends base class 'Model'.
  Types of property '$query' are incompatible.
    Type '(trx?: Transaction) => BaseQueryBuilder<this>' is not assignable to type '(trx?: Transaction) => QueryBuilderSingle<this>'.
      Type 'BaseQueryBuilder<this>' is not assignable to type 'QueryBuilderSingle<this>'.
        Types of property 'then' are incompatible.
          Type '<TResult1 = this[], TResult2 = never>(onfulfilled?: (value: this[]) => TResult1 | PromiseLike<TRe...' is not assignable to type '<TResult1 = this, TResult2 = never>(onfulfilled?: (value: this) => TResult1 | PromiseLike<TResult...'.
            Types of parameters 'onfulfilled' and 'onfulfilled' are incompatible.
              Types of parameters 'value' and 'value' are incompatible.
                Type 'this[]' is not assignable to type 'this'.
npm ERR! Test failed.  See above for more details.

@elhigu
Copy link
Contributor Author

elhigu commented Aug 21, 2017

This actually worked... just had to write interface declaration for QueryBuilderSingle mode of subclassed query builder.

import { Model, QueryBuilder, QueryBuilderBase, Transaction } from 'objection';

class BaseQueryBuilder<T> extends QueryBuilder<T> {
  session(session: any) {
    return this.mergeContext({ session });
  }
}

interface BaseQueryBuilderSingle<T> extends QueryBuilderBase<T>, Promise<T> {
  session(session: any): this;
}

// try to override $query() method to return correct query builder
class BaseModelDollarQueryOverrideTest extends Model {
  static QueryBuilder = BaseQueryBuilder;
  static RelatedQueryBuilder = BaseQueryBuilder;

  $query(trx?: Transaction): BaseQueryBuilderSingle<this> {
    return <any> this.$query(trx);
  }
}

// try to override static query() method to return correct query builder
class BaseModelStaticQueryOverrideTest extends Model {
  static QueryBuilder = BaseQueryBuilder;
  static RelatedQueryBuilder = BaseQueryBuilder;

  static query<T>(trx?: Transaction): BaseQueryBuilder<T> {
    return <any> super.query(trx);
  }
}

const testDollar = new BaseModelDollarQueryOverrideTest();
testDollar.$query().session({});

BaseModelStaticQueryOverrideTest.query().session({});

@mceachen
Copy link
Collaborator

mceachen commented Oct 12, 2017

Once TypeScript supports generics with defaults (see the issue and pr), we may be able to capture the custom query builder signature into the Model generic, and not break the current typing API.

Edit: huh, it did get merged in 2.3, I'd missed that! I'll take another look at this when I get a chance.

@brianphillips
Copy link
Contributor

@mceachen did you ever come back to looking at this and if so, have any success?

@mceachen
Copy link
Collaborator

I didn't. If you have time, I'd be happy to help review a PR though.

@jtlapp
Copy link
Contributor

jtlapp commented Dec 18, 2017

I need this fix as well and have been looking over how to do it. I have a proposal, but these typedefs are pretty complex, and I don't know if it'll work or how much reworking it requires.

Right now, as best I can make sense of things, the model class T and the query builder class QueryBuilder are both assigned via either query() or $query().

What if we instead assign them via Model's static QueryBuilder getter? That way the type of the query builder would be the type of value returned, whatever it happens to be, subject to the constraint that it extends QueryBuilder?

I'm looking into this now, but my minimal experience with Typescript type decls is making this a challenge.

UPDATE: "Accessors can't be declared in an ambient context." If only this had been a method!

OOPS: Okay, now I'm realizing that it's the call to Model.query() that yields the type assignment, so this wouldn't have worked anyway. Back to the drawing board...

@jtlapp
Copy link
Contributor

jtlapp commented Dec 18, 2017

This is not a long-term solution, or a pretty one, but it does work. I'm sharing it in case it triggers better ideas in others.

// typings
query<QB extends QueryBuilder<M>>(trxOrKnex?: Transaction | knex): QB;
static query<T, QB extends QueryBuilder<T>>(this: Constructor<T>, trxOrKnex?: Transaction | knex): QB;
// usage
return Model.query<ExtendedQueryBuilder<M>>().customMethod().insert(obj);

Surprisingly, no default generic is required to call query() or $query() using QueryBuilder. This works fine with the above typings:

// usage
return Model.query().insert(obj);

@mceachen
Copy link
Collaborator

mceachen commented Dec 18, 2017 via email

@jtlapp
Copy link
Contributor

jtlapp commented Dec 18, 2017

Okay, I'll condense my project for us to use. I was meaning to share it in the end anyway, as I think I've found a pretty nice way to encapsulate Objection in Typescript for dependency injection. I'll post it as an example in a new GitHub repository. It'll take me a few to make it presentable.

Question: So far I've found it easiest to play with the ambient objection typings locally in my own project, instead of linking my package to a local mod of the objection repo. Should I just post that? That would give us a separate copy to play with, anyway.

@jtlapp
Copy link
Contributor

jtlapp commented Dec 18, 2017

Or maybe I should post it to my local objection repo as a koa-ts-di example, for possible PR later.

I want to work with my solution for encapsulating Objection (for DI) because it may create issues for the custom query builder solution. My encapsulation itself uses generics.

@jtlapp
Copy link
Contributor

jtlapp commented Dec 19, 2017

I opted to create a working Typescript version of @koskimas's custom QueryBuilder found in examples/plugin. The README should get you started.

It was quite a bear to carry forward the generality of the plugin, but I finally succeeded.

@jtlapp
Copy link
Contributor

jtlapp commented Jan 25, 2018

I've spent several hours on this. I do not believe it is possible to provide Typescript support for custom query builders, because Typescript does not allow for parameterizing static class members.

Also, in order to avoid the impossibility of both the model being parameterized with the query builder and the query builder being parameterized with the model, the typings would have to factor out non-standard query builder methods, pass them in via a type parameter, and then &-add the factored-out methods to each occurrence of QueryBuilder within the typings.

That's a ridiculous amount of change to only partially support custom query parameters, so I didn't bother trying that. Instead, the best available solution seems to be to force the client to type-parameterize all calls to query() and $query() -- which is quite verbose.

The linked-to typings pass all existing typescript tests, but I'm not sure we want them. Plus, they only provide immediately-following methods in the query builder chain access to the custom query builder methods; the custom methods cease to be available after the first call to a built-in method.

I'm moving on and planning to avoid custom query builders as much as possible in Typescript.

@jtlapp
Copy link
Contributor

jtlapp commented Jan 25, 2018

Ah poo. I'm now looking at @elhigu's above approach to doing this using method overriding declaration-merging. I should try that with the new result-type pass-through typings.

That won't work on the generic plugin example I pointed-to, but it might for hard-coded class names. Plus I can try declaration-merging.

@jtlapp
Copy link
Contributor

jtlapp commented Jan 25, 2018

On second thought, the problem with method-overloading or declaration-merging is that the custom query builder type is only available to the methods called immediately after the overloaded or merged methods. So we'd always have code of the form:

const result = MyModel.query().specialMethod(specialParams).builtIn1().builtIn2();

In which case we might as well just write our own combined method and not bother trying to return a special query builder type:

const result = MyModel.specialQuery(specialParams).builtIn1().builtIn2();

It's shorter, and there's no need to try to return the new query builder type.

I'm thinking it could be something like this:

class MyModel extends Model {
  static contextQuery(ctx: MyAppContext) {
    return MyModel.query(ctx.knex).mergeContext({ session: ctx.session });
  }
}

And we get the same job done without a custom query builder.

@jtlapp
Copy link
Contributor

jtlapp commented Jan 25, 2018

Final thought. This three-part solution would make the custom query builder type available to all methods in the chain:

  1. Have the custom query builder implement an interface for its non-objection methods.
  2. Implement a static method on the model that returns the custom query builder, whether by overriding Model.query() or providing an alternative method.
  3. Parameterize Model with the interface and add the interface to each occurrence of the QueryBuilder return type in the chain (modifying nearly every occurrence of QueryBuilder in the typings).

@newhouse
Copy link
Contributor

God Bless you, JTL. I think it would be fun to hang out with you. 🍻I didn’t read any of this but it’s great

@mceachen
Copy link
Collaborator

@newhouse ask him about spiders

@jtlapp
Copy link
Contributor

jtlapp commented Jan 25, 2018

I didn’t read any of this but it’s great

Hilarious! I advise keeping things that way.

I'm mostly just trying to grok Typescript.

@newhouse
Copy link
Contributor

@mceachen that sounds risky! Maybe in person with @jtlapp one day... I was drinking last night when I wrote that, but you guys are awesome...

@Cmacu
Copy link

Cmacu commented Feb 19, 2018

I've been trying to implement the custom query builder with Typescript for a several days without success. I was starting to think, that I am doing something wrong until I found this issue.

@elhigu
Copy link
Contributor Author

elhigu commented Jul 11, 2019

So did you try that this compiles?

const query = Person.query().where(builder => builder.customMethod());

IIRC typings did declare those builders to be normal query builders and not the custom ones.

@koskimas
Copy link
Collaborator

Obviously we'd need to fix those typings 😄 None of this works with the current typings. My point was to change the current typings along these lines. I'll write an example for you that takes query builders.

@elhigu
Copy link
Contributor Author

elhigu commented Jul 11, 2019

Sounds really good :)

@koskimas
Copy link
Collaborator

koskimas commented Jul 11, 2019

Here you go:

// Objection types:

interface VoidCallback<T> {
  (t: T): void
}

interface Where<QB> {
  (): QB;
  (cb: VoidCallback<QB>): QB;
  // QueryBuilder<any> is correct typing here and not a workaround
  // because subqueries can be for any table (model).
  <QB2 extends QueryBuilder<any>>(qb: QB2): QB;
}

class QueryBuilder<M extends Model> extends Promise<M[]> {
  where: Where<this>;
}

interface ModelClass<M> {
  new (): M;
}

class Model {
  static query<QB extends QueryBuilder<M>, M extends Model>(this: ModelClass<M>): QB {
    return {} as QB;
  }

  $query<QB extends QueryBuilder<this>>(): QB {
    return {} as QB;
  }
}

// Your types:

class CustomQueryBuilder<M extends Model> extends QueryBuilder<M> {
  customMethod(): this {
    return this;
  }
}

class Person extends Model {
  static query<QB = CustomQueryBuilder<Person>>(): QB {
    return super.query() as any;
  }

  $query<QB = CustomQueryBuilder<Person>>(): QB {
    return super.$query() as any;
  }
}

async function main() {
  const query = Person.query().where().customMethod().where().customMethod();
  takesPersons(await query)

  const persons = await query;
  await persons[0]
    .$query()
    .customMethod()
    .where(qb => {
      qb.customMethod().where().customMethod()
    })
    .where(Person.query().customMethod())
    .customMethod()
}

function takesPersons(persons: Person[]) {

}

@koskimas
Copy link
Collaborator

I don't really know if we'd be able to fit all other types on top of this kind of types though... I think we'd need to simplify the types in other ways too. For example, currently QueryBuilder takes three generic type arguments. IIRC that's just a workaround for allowing the methods to be in any order. For example returning coming as the first method or the last. If we made some limitations to the order methods can be chained in typescript, we could maybe simplify them. I dunno.

@elhigu
Copy link
Contributor Author

elhigu commented Jul 11, 2019

If it provides better typings in other ways, then I suppose it could be good compromise. Also currently one has to decide themselves which part of chain to put to last to get correct result type (or at least earlier one had to do that... Its over a year since the last time I used objection typings).

@koskimas
Copy link
Collaborator

koskimas commented Jul 11, 2019

Aaand one more update for reference for future discussions. I got the return types working with couple of generics tricks. I also added two iterfaces StaticQueryMethod and QueryMethod that make it much easier to start using custom query builders.

// Objection types:

interface CallbackVoid<T> {
  (arg: T): void
}

type ItemType<T> = T extends Array<infer R> ? R : T;
type ArrayType<T> = T extends Array<any> ? T : Array<T>

interface WhereMethod<QB> {
  (): QB;
  (cb: CallbackVoid<QB>): QB;
  <QBA extends QueryBuilder<any>>(qb: QBA): QB;
}

class QueryBuilder<M extends Model, R = M[]> extends Promise<R> {
  where: WhereMethod<this>;
}

interface DefaultStaticQueryMethod extends StaticQueryMethod<any> {
  <M extends Model, QB extends QueryBuilder<M>>(this: ModelClass<M>): QB
}

interface StaticQueryMethod<QB extends QueryBuilder<any, any>> {
  (): QB
}

interface QueryMethod<QB extends QueryBuilder<any, any>> {
  (): QB
}

interface ModelClass<M> {
  new (): M;
}

class Model {
  static query: DefaultStaticQueryMethod;
  $query: QueryMethod<QueryBuilder<this, this>>
}

// Your types:

class AnimalQueryBuilder<M extends Model, R = M[]> extends QueryBuilder<M, R> {
  customMethod(): this {
    return this;
  }
}

class Person extends Model {
  firstName: string;
}

class Animal extends Model {
  name: string;

  static query: StaticQueryMethod<AnimalQueryBuilder<Animal>>
  $query: QueryMethod<AnimalQueryBuilder<this, this>>
}

async function basic() {
  const query = Person
    .query()
    .where(Person.query())
    .where(qb => {
      qb.where()
    })

  const persons = await query;
  console.log(persons[0].firstName);

  const person = await persons[0]
    .$query()
    .where(Person.query())
    .where(qb => {
      qb.where()
    })

  console.log(person.firstName);
}

async function custom() {
  const query = Animal
    .query()
    .customMethod()
    .where(Animal.query().where().customMethod())
    .where(qb => {
      qb.customMethod().where()
    })

  const animals = await query;
  console.log(animals[0].name)

  const animal = await animals[0]
    .$query()
    .customMethod()
    .where(qb => {
      qb.customMethod().where().customMethod()
    })
    .where(Animal.query().customMethod())
    .customMethod()

  console.log(animal.name)
}

@koskimas
Copy link
Collaborator

koskimas commented Jul 11, 2019

Okay, that didn't work with methods that change the return type of the query builder. I believe this is impossible to solve with typescript right now... Only way I figured out is to pass around SEVEN generic types everywhere. It would be a nightmare to use and maintain. I'm giving up for now.

We would need a generic meta function like this:

// objection types

interface QueryBuilder<M extends Model, R> {}
type ChangeSecondGenericArgType<QB, T> = <MAGIC>

// project types

class CustomQueryBuilder<M extends Model, R> extends QueryBuilder<M, R> {}

const q: ChangeSecondGenericArgType<CustomQueryBuilder<Person, Person[]>, number> = new CustomQueryBuilder<Person, number>()

@koskimas
Copy link
Collaborator

koskimas commented Jul 12, 2019

I think I just woke up with a solution 😄 At least for the changing return types, but I think this could be the key for the whole problem.

interface CallbackVoid<T> {
  (arg: T): void;
}

interface RawBuilder {
  as(): this;
}

interface LiteralBuilder {
  castText(): this;
}

interface ReferenceBuilder {
  castText(): this;
}

function raw(sql: string, ...bindings: any): RawBuilder {
  return notImplemented();
}

type AnyQueryBuilder = QueryBuilder<any, any>;
type Raw = RawBuilder;
type Operator = string;
type NonLiteralValue = Raw | ReferenceBuilder | LiteralBuilder | AnyQueryBuilder;
type ColumnRef = string | Raw | ReferenceBuilder;

type Value =
  | NonLiteralValue
  | string
  | number
  | boolean
  | Date
  | string[]
  | number[]
  | boolean[]
  | Date[]
  | null
  | Buffer;

/**
 * Type for keys of non-function properties of T.
 */
type NonFunctionPropertyNames<T> = { [K in keyof T]: T[K] extends Function ? never : K }[keyof T];

/**
 * Given a model type, returns the equivalent POJO type.
 */
type Pojo<T> = { [K in NonFunctionPropertyNames<T>]: T[K] | NonLiteralValue };

/**
 * Just like Pojo<M> but all properties are optional.
 */
type PartialPojo<M> = Partial<Pojo<M>> & object;

/**
 * Extracts the model type from a query builder type QB.
 */
type ModelType<QB> = QB extends QueryBuilder<infer M> ? M : never;

/**
 * Extracts the property names of the query builder's model class.
 */
type ModelProps<QB> = Exclude<NonFunctionPropertyNames<ModelType<QB>>, 'QB'>;

/**
 * Gets the single item query builder type for a query builder.
 */
type SQB<QB extends AnyQueryBuilder> = QB['SQB'];

/**
 * Gets the multi-item query builder type for a query builder.
 */
type AQB<QB extends AnyQueryBuilder> = QB['AQB'];

/**
 * Gets the number query builder type for a query builder.
 */
type NQB<QB extends AnyQueryBuilder> = QB['NQB'];

interface WhereMethod<QB extends AnyQueryBuilder> {
  // These must come first so that we get autocomplete.
  (col: ModelProps<QB>, op: Operator, value: Value): QB;
  (col: ModelProps<QB>, value: Value): QB;

  (col: ColumnRef, op: Operator, value: Value): QB;
  (col: ColumnRef, value: Value): QB;

  (condition: boolean): QB;
  (cb: CallbackVoid<QB>): QB;
  (raw: Raw): QB;
  <QBA extends AnyQueryBuilder>(qb: QBA): QB;

  (obj: PartialPojo<ModelType<QB>>): QB;
  // We must allow any keys in the object. The previous type
  // is kind of useless, but maybe one day vscode and other
  // tools can autocomplete using it.
  (obj: object): QB;
}

interface WhereRawMethod<QB extends AnyQueryBuilder> {
  (sql: string, ...bindings: any): QB;
}

interface WhereWrappedMethod<QB extends AnyQueryBuilder> {
  (cb: CallbackVoid<QB>): QB;
}

interface WhereExistsMethod<QB extends AnyQueryBuilder> {
  (cb: CallbackVoid<QB>): QB;
  (raw: Raw): QB;
  <QBA extends AnyQueryBuilder>(qb: QBA): QB;
}

interface WhereInMethod<QB extends AnyQueryBuilder> {
  (col: ColumnRef | ColumnRef[], value: Value[]): QB;
  (col: ColumnRef | ColumnRef[], cb: CallbackVoid<QB>): QB;
  (col: ColumnRef | ColumnRef[], qb: AnyQueryBuilder): QB;
}

interface FindByIdMethod<QB extends AnyQueryBuilder> {
  (id: number): SQB<QB>;
}

class QueryBuilder<M extends Model, R = M[]> extends Promise<R> {
  AQB: QueryBuilder<M, M[]>;
  SQB: QueryBuilder<M, M>;
  NQB: QueryBuilder<M, number>;

  where: WhereMethod<this>;
  andWhere: WhereMethod<this>;
  orWhere: WhereMethod<this>;
  whereNot: WhereMethod<this>;
  andWhereNot: WhereMethod<this>;
  orWhereNot: WhereMethod<this>;

  whereRaw: WhereRawMethod<this>;
  orWhereRaw: WhereRawMethod<this>;
  andWhereRaw: WhereRawMethod<this>;

  whereWrapped: WhereWrappedMethod<this>;
  havingWrapped: WhereWrappedMethod<this>;

  whereExists: WhereExistsMethod<this>;
  orWhereExists: WhereExistsMethod<this>;
  whereNotExists: WhereExistsMethod<this>;
  orWhereNotExists: WhereExistsMethod<this>;

  whereIn: WhereInMethod<this>;
  orWhereIn: WhereInMethod<this>;
  whereNotIn: WhereInMethod<this>;
  orWhereNotIn: WhereInMethod<this>;

  findById: FindByIdMethod<this>;
}

interface StaticQueryMethod {
  <M extends Model>(this: ModelClass<M>): AQB<M['QB']>;
}

interface QueryMethod {
  <M extends Model>(this: M): SQB<M['QB']>;
}

interface ModelClass<M> {
  new (): M;
}

class Model {
  QB: QueryBuilder<this, this[]>;

  static query: StaticQueryMethod;
  $query: QueryMethod;
}

// Your types:

class CustomQueryBuilder<M extends Model, R = M[]> extends QueryBuilder<M, R> {
  AQB: CustomQueryBuilder<M, M[]>;
  SQB: CustomQueryBuilder<M, M>;
  NQB: CustomQueryBuilder<M, number>;

  customMethod(): this {
    return notImplemented();
  }
}

class BaseModel extends Model {
  QB: CustomQueryBuilder<this>
}

class Person extends Model {
  firstName: string;
}

class Animal extends BaseModel {
  name: string;
  species: string;
}

function notImplemented<T>(): T {
  return {} as any;
}

async function testWhereTypes() {
  type QB = QueryBuilder<Person, Person[]>;

  takes<QB>(
    Person.query().where('firstName', 'Sami'),
    Person.query().where('age', '>', 10),
    Person.query().where(Person.query()),
    Person.query().where(Animal.query()),
    Person.query().where(true),
    Person.query().where({ a: 1 }),
    Person.query().where(raw('? = ?', [1, '1'])),

    Person.query().where(qb => {
      takes<QB>(qb);
      qb.whereIn('id', [1, 2, 3]);
    }),

    Person.query().where(function() {
      takes<QB>(this);
      this.whereIn('id', [1, 2, 3]);
    })
  );

  const persons = await Person.query().where('id', 1);
  takes<Person[]>(persons);
}

async function custom() {
  const query = Animal.query()
    .customMethod()
    .where(
      Animal.query()
        .where({ name: 'a' })
        .customMethod()
    )
    .where(qb => {
      qb.customMethod().where('name', 'Sami');
    });

  const animals = await query;
  console.log(animals[0].name);

  const animal = await animals[0]
    .$query()
    .customMethod()
    .where(qb => {
      qb.customMethod()
        .where('a', 1)
        .customMethod();
    })
    .where(Animal.query().customMethod())
    .customMethod();

  console.log(animal.name);
}

function takes<T>(...value: T[]): T[] {
  return value;
}

So the key is to add those three subtypes MQB, SQB and NQB for the custom query builder and the QB type for each model, and that's it! See Animal and CustomQueryBuilder in the example above.

@koskimas
Copy link
Collaborator

koskimas commented Jul 12, 2019

Good news is that it all seems to work. The bad news is that the types need to be completely rewritten.

So for anyone (most people) that didn't read my comment diarrhea, this is how you would create a custom query builder after the types have been rewritten:

class CustomQueryBuilder<M extends Model, R = M[]> extends QueryBuilder<M, R> {
  // Unfortunately these three types need to be hand-written 
  // for each custom query builder.
  AQB: CustomQueryBuilder<M, M[]>
  SQB: CustomQueryBuilder<M, M>
  NQB: CustomQueryBuilder<M, number>

  customMethod(): this {
    return this;
  }
}

class BaseModel extends Model {
  static QueryBuilder = CustomQueryBuilder;
  // Unfortunately this is needed in addition to the `QueryBuilder` property.
  QB: CustomQueryBuilder<this>
}

class Person extends BaseModel {
  firstName: string;
  lastName: string;
}

The custom query builder type is correctly inherited over any method call, callback, subquery, etc. With the new types you even get autocomplete for properties when you start to type stuff like .where('. I think the new types will be much cleaner too since we don't need to drag around three template arguments. Instead, we only drag around one, the query builder type.

@falkenhawk What's with the eyes? 😄

@falkenhawk
Copy link
Contributor

Those are my eyes at the moment. Wide-open in amazement

@koskimas
Copy link
Collaborator

The new typings are being worked on in this branch.

@koskimas
Copy link
Collaborator

koskimas commented Jul 24, 2019

Okay, the tricky parts of the typings are now ported to the new structure. There's still a lot of things to do to make them releasable (adding all methods the old typings had, adding tests, trying them out on real projects, etc). I won't be doing that anymore before 2.0 is out. I've already spent a week on this, when I was meant to be getting objection ready for 2.0. If you want the new typings for 2.0, help is needed.

@willsoto
Copy link
Contributor

@koskimas Will the new typings be merged into 2.0 and then incrementally added to or are you going to hold off on merging that branch until the typings are in a more complete state?

@koskimas
Copy link
Collaborator

@willsoto The tests need to pass before it can be merged into 2.0

@jtlapp
Copy link
Contributor

jtlapp commented Jul 29, 2019

It's been over a year since I've had to do anything server-side, and I'm starting a new project, but this time I'm going to find a way to do without Objection, as much as I like it. I need a solution that prioritizes Typescript -- no builds without accurate types. Otherwise I'd offer to help with @koskimas's QueryBuilder support proposal.

@koskimas
Copy link
Collaborator

koskimas commented Jul 30, 2019

@jtlapp Don't get me wrong, there's absolutely no obligation to help and there's already two lovely volunteers that offered their help, so we don't even need it at this point.

But I fail to see the logic in your comment. You are saying you won't use objection because it doesn't prioritize typescript and use that as a reason to not help us prioritize typescript in objection?

@jtlapp
Copy link
Contributor

jtlapp commented Jul 30, 2019

I think prioritizing Typescript means committing to never releasing a feature unless it supports Typescript static type checking to the maximum degree possible in Typescript.

That means redesigning code to make static type checking possible in Typescript, not to the degree that existing Javascript permits, but to the degree that it is possible to do in Typescript. It starts with thinking, "What can Typescript do to help?" and working backwards from there.

I doubt there are many Javascript modules that adopt this principle. Usually when Typescript is valued enough to do this, the author will have written (or rewritten) the module in Typescript.

We may need a native Typescript equivalent of Objection. I betcha this would open up all sorts of syntax efficiencies not currently available, in addition to better type support. I wouldn't be hesitating then.

@mordof
Copy link

mordof commented Jul 30, 2019

I just can't agree with that perspective, for a number of reasons. I have three main points that I'll list to keep it succinct:

  1. those expectations are extremely high for basically any project in existence, and yet this is open source. When something isn't absolutely mission-critical with billions of dollars involved, it very rarely has that level of scrutiny.
  2. typescript doesn't have enough capability to handle some of the typing structures needed that are well suited for ORMs (i don't agree with abandoning patterns in javascript that are a good fit, because the typing system can't handle it, especially when the types can be "good enough")
  3. Personally, I'd much rather have a solid ORM which is good at run-time, and I can be confident about that aspect, rather than focusing on the types as much. Fully typed libraries are great and i'd love an ORM to exist that you describe, but I came from TypeORM; it has more detailed types, but it's also a giant mess with some rather large runtime bugs that are completely unnacceptable for me.

As a result of those three, that's why I'm putting effort into helping with Objections types primarily - I want to get them to a state where people can be confident about their correctness/robustness, as much as is possible with what typescript offers. My goal is to have all of the happy path covered correctly; anything not in the happy path I'm trying to get as much as is possible handled by types as well. Things like what functions can be called, at what time, on the query builder are not something that can be accounted for at this time, for example.

@jtlapp
Copy link
Contributor

jtlapp commented Jul 30, 2019

That's a good response @mordof. Thanks for the perspective. Here's a little more background.

I'm the one who redid the Objection typings to provide return types, but which apparently I did in a way that was incompatible with query builders.

I developed a server with those typings and found myself not needing the query builder after all. But I was still frustrated type lossiness and wondering all the way what would be possible were Objection written in Typescript with the intent of preserving type. (Unfortunately, I do not recall the issues I was facing at the time and so have no examples for you here.)

I may yet be back to help with Objection, but I'm got a few other avenues to explore first. I do know better than to touch TypeORM.

@mordof
Copy link

mordof commented Jul 30, 2019

I'm the one who redid the Objection typings to provide return types,

Awesome :) didn't know that.

but which apparently I did in a way that was incompatible with query builders.

To be honest, I have yet to find a way that actually is correctly compatible with query builders. Every approach I've tried has had some problem or another, so the latest approach has some rigidity to it which is "kind of" matching the actual query builder. (Edit: Most of the new-typings progress has been @koskimas's efforts. I've spent time trying to adjust them and expand on it, but he's done a great job with the updates, and I've yet to find a good way to resolve the complications either.)

Especially because typescript has released some features only very recently that are bordering crucial to pull that off properly, I'd wager the complications being tackled now probably have less to do with your choices, and more to do with the lack of choices in general.

I developed a server with those typings and found myself not needing the query builder after all.

So you didn't need any .where() .findById() or any function basically from knex? I feel like I'm misunderstanding what you mean by that, because if that's actually true, then I'm having trouble understanding why you'd even bother using Objection, as that's basically all Objection is.

But I was still frustrated type lossiness and wondering all the way what would be possible were Objection written in Typescript with the intent of preserving type.

That's a fair point. Thinking back on my experiences with ORMs, I personally prefer query builders the most. That in itself is the problem for Objection types (bad typescript querybuilder capability). There are potentially other solid options for ORMs that don't use a query builder, but I stopped looking after objection, so I can't help you there.

I may yet be back to help with Objection, but I'm got a few other avenues to explore first.

Always good to explore your options 👍. It's definitely crucial to use the right tool for the job. If Objection isn't it, that's perfectly fine... though it's always sad to see people go.

I do know better than to touch TypeORM.

🎊 👍 💯 I'm really happy to hear that. It's such a nightmare 😆

With all that said, I wish you happy hunting, and hope you find what you're looking for. Also if you do happen to find a query builder ORM that actually has reliable types around it, feel free to send me a link! I'd love to find an example to learn from.

@jtlapp
Copy link
Contributor

jtlapp commented Jul 30, 2019

I developed a server with those typings and found myself not needing the query builder after all.

So you didn't need any .where() .findById() or any function basically from knex? I feel like I'm misunderstanding what you mean by that, because if that's actually true, then I'm having trouble understanding why you'd even bother using Objection, as that's basically all Objection is.

Oops, sorry, I meant that I didn't need custom query builders, which is thing we couldn't type.

@jtlapp
Copy link
Contributor

jtlapp commented Jul 30, 2019

And I should mention that I had a lot of assistance from @mceachen, helping me to get unstuck and fixing my mistakes, because he had a better grasp of Typescript typings than I.

@mordof
Copy link

mordof commented Jul 30, 2019

@jtlapp ooh ok. That part has viable types now 🙂 it's in the new-typings branch, and is being worked on get those covering everything (at minimum) that was previously typed as well.

Once that process is done, I'm going to be reviewing absolutely all of the types and ensuring they're as complete/compatible as they can be. If there's viable changes to certain areas (e.g. some helpers to define properties on a model) for the actual js that can help improve/solidify the types, without taking away from the core style of objection, i'll be recommending/making PRs for those as well.

@mordof
Copy link

mordof commented Jul 30, 2019

I do think it's possible to get close to what you're hoping Objection can be, however I acknowledge that will take time, and you may not be able to keep with Objection during that time. That's ok too.

@jtlapp
Copy link
Contributor

jtlapp commented Jul 30, 2019

I've got some experiments planned should either lead me to the right solution or else send me back here a bit smarter to help.

@koskimas
Copy link
Collaborator

We can finally close this one. Fixed in v2.0 branch. Soon to be released

@Dakuan
Copy link

Dakuan commented Feb 24, 2021

@koskimas thanks very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests