Make function types generic in type definitions

[lorefnon]

No description provided.

[kibertoad]

Impressive! What makes it WIP?

[lorefnon]

@kibertoad Thanks for taking a look so quickly. I have outlined on the motivation of my change in #3156 and would like to have better tests after receiving some feedback (hence the WIP).

[pvider]

@lorefnon Thank you very much for this commit, I did some tests and it's so nice to have types in query results. However, I found a bug in type inference for .select. Select query as I know should return array of objects but it returns an intersection type any[] & Pick<any, 'columnNames...'>. Should it return Pick<any, 'columnNames...'>[] for .select instead? For example now I have:

    const results = await knex('Users').select(['id', 'isMaster']) // results has wrong type any[] & Pick<any, "id" | "isMaster">
    results.isMaster // bug: this property 'isMaster' exists
    results[1].qwerty // bug: elements of array have <any> type

I expect results type to be Pick<any, 'columnNames...'>[] if possible.

[lorefnon]

@pvider Thanks for taking a look. This PR is still early stage and will take some time to stabilize.

I have now added tests for most common crud scenarios.

The autocompletion for single-table operations should be much better now:

Auto completion of column name in where:

Type safety in column values (number type inferred for id attribute):

Type safety in update argument:

[lorefnon]

@pvider @kibertoad I believe this is now adequately tested. Please let me know if you have any comments.

[pvider]

@lorefnon I did some tests in VSC through pure JS with JSDOC and should note it works really well. My test results are same as yours. This is really powerful addition to knex even for pure-JS-users like me, so I want to share the following snippet for those who don't need TypeScript yet, but wants same power via JSDOC (works from the box in VSC):

/**
 * @typedef {Object} User
 * @property {number} id
 * @property {number} age
 * @property {string} name
 * @property {boolean} active
 * @returns {import('knex-generic-type-defs').QueryBuilder<User, {}>}
 */
const Users = () => knex('Users')

The above typedef is analogue of this TypeScript interface:

interface User {
  id: number;
  age: number;
  name: string;
  active: boolean;
}

Next we define returned type via @returns {import('knex-generic-type-defs').QueryBuilder<User, {}>} with proper import ('knex-generic-type-defs') and type variable (User) from typedef.
In the end we write one line of real JS-code const Users = () => knex('Users') to make table helpers.
Use them instead of knex('Users') to get type inference:

// infered type:
// Pick<User, "id">[]
const result = await Users().select('id')

[lorefnon]

@pvider Thanks for trying this out. If this PR gets merged, we can submit another to add your example to the docs - I am sure it will benefit many people.

[kibertoad]

Thanks a lot!

[Xiphe]

Thanks a lot for investing time into making the TS integration even better! Where can I find docs for this and how would I migrate from 0.16.5 to 0.16.6?

[loxs]

I'm using Knex in TypeScript and till now enforced return types via io-ts.
After upgrading to latest, now I get all kinds of funky errors and I don't even know where to begin fixing things.

Documentation on how to use this would be greatly appreciated. The few examples I found in the discussions about the linked issues and pull requests (like the example of knex<Book, Book[]>) are not good for me as I am using knex in a dynamic fashion where query is generated from config and neither the table name, nor the columns are known in advance.

[felixmosh]

@lorefnon thank you for this PR.

It just not works,

I'm on the latest TS (3.4.5)

What it can be? I've wasted 4 hours today to figure out how to make it work, without any success.

[Xiphe]

created an issue for this the above stuff: #3193

[kibertoad]

@loxs Is 0.16.7 not working for you either? From my understanding it was supposed to be using any everywhere by default and as such not be breaking. If it's still problematic for you, I'm probably going to unpublish 0.16.6 and 0.16.7 and release it as next instead.

[loxs]

@kibertoad It's not working for me and I still can't figure out why. Had to revert the update to latest, will probably give it another go tomorrow.

[lorefnon]

Ok, the impact of this change was much more severe than I originally anticipated. I apologize for the wasted time and effort here - this could certainly have been managed better.

@kibertoad Let me know if you'd like me to send a PR that reverts the changes. Unpublishing generally results in a poor experience for people (esp. corporations) who use npm proxies because afaik popular solutions like verdaccio don't handle unpublishing well and people continue to receive unpublished code.

So the safest thing to do here might be to republish another minor version after reverting the changes.

I can continue iterating on the ideas here in a branch (or a personal fork) and submit a PR once I have a better understanding of all usage patterns (which, as has now been proven, I don't at the moment).

@felixmosh This particular example will not work because of contentId as id usage.

So the general idea is that inference works as long as the things being selected are keys of the Record type. But the moment we find usage like "contentId as id" typescript inference can no longer be relied upon because typescript doesn't provide a way to extract types from substring. So we fallback to any because anything else would be incorrect.

However the columns function is generic, so it could be provided with the type of the result:

db<Seo>
    .columns<{
        id: string; 
        type: string; 
        url: string; 
        prevUrl: string
    }>('contentId as id', 'type', 'url', 'prevUrl');

Once you specify a result type it would be retained down the fluent chain.

@loxs This is interesting usage. If you could share some snippets of your usage, I would be happy to check on this further. Given io-ts uses very similar inference based approach to what I was using in this PR, it should be possible to extract static types from your io-ts runtime types and use them in the generic parameters to Knex functions.

[loxs]

@lorefnon yeah, I'll try to give a nice example when I have some time. I am also willing to participate in testing with new pull requests etc. Please, keep me in the loop.

[kibertoad]

@lorefnon Valid point about proxies, but I still would prefer not to revert the change, but iterate over it and also create some documentation instead. In my experience success rate of long-running branches or forks for developing complex features is really low, and I would expect feature just to die off if we follow that route. I believe that lack of proper type safety is one of major drawbacks of knex as it is now, so it is well worth the gamble. Would be awesome if we all could come together and iterate over a series of -next release to make sure that transition is as painless and covers as much ground as possible.
Will release "next" version in about 5 hours.

[lorefnon]

Ok alright. Sounds good to me.

[kibertoad]

0.17.0-next is now out. Suggestions, problem reproductions and PRs most welcome!

[kibertoad]

0.17.0-next2 is out and should be more of a drop-in replacement. @loxs @Xiphe @felixmosh Can you give it a try and see if you are having any problems with it?

[lorefnon]

I'll work on adding some documentation here, but in the meanwhile this test file can be used as a reference for patterns which are known to be type safe and APIs which are now generic (to aid in intellisense).

Also, as @kibertoad mentioned, we don't expect any existing usage to break compilation (the result can of course be any[]).

While I am still in process of parallely expanding the tests to cover all usage patterns, given the large surface area of this library (and me not having indepth familiarity with the source) some aspects may get missed and I'll be happy to look closer into any snippets which fail to compile.

[felixmosh]

Thank you for the work, there are some use cases that it still not works:

1. usage of .first() still return a array
   
2. there is an error with ChainableInterface,
   
3. .insert() return the wrong value, it suppose to return an array with one element (the insert id).
   
4. usage of .modify(cb), cb, used to be annotated as function that gets 2 params, querybuilder and user params, not it gets 3, this, queryBuilder, user params. is that correct? or a type issue?
5. usage of .map(),
   

I must admit that this version (0.17.0-next2) has much less type errors then previous one, kudos!

[loxs]

Hi, next2 is better for sure, as probably half of the problems are gone.

Here is an attempt to explain my use case and question on how would one go to make it compile.

I start building a query in one function, then pass the intermediate to another, then to another. Logic branches at some points and finally I execute it with await.

Till now I only annotated my functions like

const addWhereClause = (intermediateQuery: Knex, more, args): Knex => {
  return knex.where("of course with some logic")
}

I finally do something like this (decoding with io-ts):

  const sqlRes = await query;
  const result = decodeSqlResultSet(sqlRes, MyIOTSCodec);

When I try to compile with new Knex I get errors of this kind:

Error:(219, 5) TS2322: Type 'QueryBuilder<any, unknown> & ChainableInterface<any[]>' is not assignable to type 'WhereResult<any, (DeferredKeySelection<any, string, boolean, {}> | DeferredKeySelection<any, never, false, {}>)[]>'.
  Type 'QueryBuilder<any, unknown> & ChainableInterface<any[]>' is not assignable to type 'QueryBuilder<any, (DeferredKeySelection<any, string, boolean, {}> | DeferredKeySelection<any, never, false, {}>)[]>'.

I kind of get where is this coming from, but probably will need a lot of effort to try and make it work. Hopefully it won't require rewriting all of my query generators.

Error:(205, 5) TS2322: Type 'QueryBuilder<any, unknown> & ChainableInterface<any[]>' is not assignable to type 'WhereResult<any, (DeferredKeySelection<any, string, boolean, {}> | DeferredKeySelection<any, never, false, {}>)[]>'.
  Type 'QueryBuilder<any, unknown> & ChainableInterface<any[]>' is not assignable to type 'QueryBuilder<any, (DeferredKeySelection<any, string, boolean, {}> | DeferredKeySelection<any, never, false, {}>)[]>'.
    Types of property 'and' are incompatible.
      Type 'QueryBuilder<any, unknown>' is not assignable to type 'QueryBuilder<any, (DeferredKeySelection<any, string, boolean, {}> | DeferredKeySelection<any, never, false, {}>)[]>'.
        Type 'unknown' is not assignable to type '(DeferredKeySelection<any, string, boolean, {}> | DeferredKeySelection<any, never, false, {}>)[]'.

This one I don't get at all.

These two errors are prevalent across my whole codebase

[loxs]

OK, if I put a bunch of QueryBuilder<any, any> on my queries it seems to work.

Though this seems a bit worse than before, as previously I had this, which no more works, though I might have been wrong before. (expected by decodeSQLResultset in my previous example)

export type TGenericResultSet = Array<{ [key: string]: any }>;
// and later
const result : TGenericResultSet = await query

Probably the modern equivalent would be something like this (which still doesn't work):

const query: QueryBuilder<any, TGenericResultSet> = dbConn.select() // etc
const result = await query

This one fails with:

Error:(72, 9) TS2322: Type 'QueryBuilder<any, unknown>' is not assignable to type 'QueryBuilder<any, { [key: string]: any; }[]>'.

[kibertoad]

@felixmosh @loxs New iteration of typings is out: knex@0.17.0-next3
Hopefully it resolves some of the issues that you are experiencing.

[felixmosh]

@felixmosh @loxs New iteration of typings is out: knex@0.17.0-next3
Hopefully it resolves some of the issues that you are experiencing.

I will try it tomorrow, thanks!

[lorefnon]

@felixmosh Thanks for pointing out these issues.

usage of .first() still return a array
.insert() return the wrong value, it suppose to return an array with one element (the insert id).
usage of .map(),

These aspects will work as expected now.

there is an error with ChainableInterface

I wasn't able to reproduce this. Are you using typescript 3.2+ ?

usage of .modify(cb), cb, used to be annotated as function that gets 2 params, querybuilder and user params, not it gets 3, this, queryBuilder, user params. is that correct? or a type issue?

Actually, there hasn't been any change here. the this argument is not a normal function argument, but is specially handled by typescript and indicates the type of this in function context. Reference.

[lorefnon]

@loxs Thanks for your comments. I have however, not been able to fully comprehend the aspects pointed out in #3168 (comment).

const addWhereClause = (intermediateQuery: Knex, more, args): Knex => {
  return knex.where("of course with some logic")
}

I believe this will work if you use Knex.QueryBuilder here in both places instead of Knex. The Knex interface is a sub-type of QueryBuilder so, QueryBuilder isn't directly assignable to it (Typescript doesn't allow implicit downcasting)

I tried this in previous version of Knex as well, and received an error there as well (I am assuming strict mode usage):

Type 'QueryBuilder' is missing the following properties from type 'Knex': VERSION, __knex__, raw, transaction, and 8 more.

I am assuming that this is a snippet extracted from a larger usage, and in the actual code something was a conversion (to any, or a cast) happening somewhere.

export type TGenericResultSet = Array<{ [key: string]: any }>;
// and later
const result : TGenericResultSet = await query

This does work (though is not type-safe) if query has not been casted to anything else before.

Probably the modern equivalent would be something like this (which still doesn't work):

const query: QueryBuilder<any, TGenericResultSet> = dbConn.select() // etc
const result = await query

This will not work. So taking a step back, one of the changes introduced here is that the Knex instance internally tracks the result type. By default the result type is unknown because it models unknown values better than any. However, when then is invoked (either through an explicit then call or implicitly by await) we return any if the result is still unknown. This makes the change less breaking so that everyone doesn't have to cast their results to any[] or something else.

But the caveat of this is that intermediate QueryBuilder instances can not be directly assigned to a QueryBuilder<any, T> where T is some type other than any or unknown.

However, all the functions where result type can change, accept a generic type parameter for directly modifying the result type. So we can write the above as:

 const query2: Knex.QueryBuilder<any, TGenericResultSet> = knex.select<TGenericResultSet>() // etc
 const result2 = await query;

And thanks to inference we can get rid of the duplication:

const query2 = knex.select<TGenericResultSet>();
const result2 = await query;

Which is exactly the same thing.

You can also assign this to Knex.QueryBuilder<any, any> because unknown is assignable to any. So this works:

const query2: Knex.QueryBuilder<any, any> = knex.select();
const result2 = await query;

If the above doesn't address your problem adequately, it would be great if you could share your typescript version, tsconfig.json and some samples which are still failing.

[loxs]

@lorefnon - yes, my code snippets above were wrong (I typed them in the GitHub interface directly). And yes, what you say makes sense and I'll play some more with my code and see if all goes well (I think it should).

I'll probably need to refactor my code a lot, but that's probably a good thing, as it will be more type safe. We'll also see how it plays with io-ts, which I probably will still want to use to really enforce the return types, even on runtime.

[loxs]

@kibertoad you have (probably by mistake) published it as @latest and not as @next

[loxs]

One bug report:

This should probably be string instead of string[]

[kibertoad]

Good catch. Will fix asap, sorry

[loxs]

Also, I totally don't get this (I made the column name to be an array just for the sake of making it not complain):

[lorefnon]

I'll probably need to refactor my code a lot, but that's probably a good thing, as it will be more type safe.

@loxs I'd like to prevent the need for unnecessary refactoring. Even if that may need some more iterations in the definitions here.

Also, I would like to clarify that these changes don't significantly improve type safety in most practical situations (when we have aliases and joins), though they make the experience of working in type aware editors better. There is nothing stopping the library consumer from passing the wrong result type as a generic param and Knex will trust it completely.

For true slick-style typesafety we would need to derive the types from the database itself and provide type-safe helpers for aliasing, but we are not quite there yet. As a part of a different project I have been working on database driven type generation but it is not in as good a state that I can extract it out and publish it as something more generic.

So yeah, I think retaining your io-ts based validations is a good idea esp. if you use encoders for datetime etc.

Your examples in #3168 (comment) and #3168 (comment) arent working because the wereIn<any, TGenericResult> is problematic. where* methods don't accept result type because adding where conditions to a query never changes the type of result. So result type can be changed using select, returning, from etc. but not where, having, orderBy etc. In general the type signature will have a type paramter TResult wherever result type can be changed.

Because of the <any, TGenericResult> type parameters being provided, ts skips over the signature that is actually applicable in your case and keeps trying other ones and eventually fails when the last one doesn't satisfy the constraints.

[felixmosh]

@felixmosh Thanks for pointing out these issues.

usage of .first() still return a array
.insert() return the wrong value, it suppose to return an array with one element (the insert id).
usage of .map(),

These aspects will work as expected now.

there is an error with ChainableInterface

I wasn't able to reproduce this. Are you using typescript 3.2+ ?

usage of .modify(cb), cb, used to be annotated as function that gets 2 params, querybuilder and user params, not it gets 3, this, queryBuilder, user params. is that correct? or a type issue?

Actually, there hasn't been any change here. the this argument is not a normal function argument, but is specially handled by typescript and indicates the type of this in function context. Reference.

I can confirm that .first, .insert works great!
The this attribute of modify callback works as well.

I still get the error that is related to ChainableInterface, i'm using the latest TS (3.4.5)

ERROR in /Users/xxx/Projects/node/xxx/api/node_modules/knex/types/index.d.ts(1328,13):
TS2430: Interface 'ChainableInterface<T>' incorrectly extends interface 'Bluebird<T>'.
  Types of property 'asCallback' are incompatible.
    Type '(callback: Function) => this' is not assignable to type '{ (callback: (err: any, value?: T) => void, options?: SpreadOption): this; (...sink: any[]): this; }'.
      Type 'this' is not assignable to type 'this'. Two different types with this name exist, but they are unrelated.
        Type 'ChainableInterface<T>' is not assignable to type 'this'.

There are still errors related to map, reduce.
For example:

const response = await db(translationsTable)
        .select('key', 'value')
        .where({ namespace: query.namespace })
        .reduce((result, { key, value }) => {
          result[key] = value;
          return result;
        }, {});

Error:(48, 30) TS2684: The 'this' context of type 'WhereResult<any, (DeferredKeySelection<any, string, true, {}, boolean> | DeferredKeySelection<any, "value" | "key", true, {}, false>)[]>' is not assignable to method's 'this' of type 'Bluebird<any[] & Iterable<any>>'.
  Types of property 'nodeify' are incompatible.
    Type '{ (callback: (err: any, value?: any[]) => void, options?: SpreadOption): this; (...sink: any[]): this; }' is not assignable to type '{ (callback: (err: any, value?: any[] & Iterable<any>) => void, options?: SpreadOption): Bluebird<any[] & Iterable<any>>; (...sink: any[]): Bluebird<any[] & Iterable<any>>; }'.
      Type 'this' is not assignable to type 'Bluebird<any[] & Iterable<any>>'.
        Type 'Bluebird<R>' is not assignable to type 'Bluebird<any[] & Iterable<any>>'.
          Type 'R' is not assignable to type 'any[] & Iterable<any>'.
            Type 'R' is not assignable to type 'any[]'.

[lorefnon]

@felixmosh Can you also please provide me outputs of npm ls bluebird and npm ls @types/bluebird ?

[loxs]

@lorefnon Thanks for the explanations. It makes more sense now.

Though I still fail to make it compile and the best thing I have so far is this:

Here is the code itself if you want to play with it:

import * as t from 'io-ts';
import {QueryBuilder} from 'knex';

type TGenericResultSet = Array<{ [key: string]: any }>;

const CCountries = t.type({
  id: t.number,
  identifier: t.string,
  name: t.string,
  currency_id: t.number,
  currency_identifier: t.string,
  currency_from_base: t.string,
  currency_to_base: t.string
});
type TCountries = t.TypeOf<typeof CCountries>;

export const decodeIOObject = <T extends {}>(
  obj: { [key: string]: any },
  codec: t.TypeC<T>
): t.TypeOf<typeof codec> => {
  const decoded = codec.decode(obj);
  if (decoded.isRight()) {
    return decoded.value;
  } else {
    console.log('Offending object:', obj);
    const errPath = PathReporter.report(decoded);
    errPath.forEach(e => console.error(e));
    throw new Error('Unexpected input object');
  }
};

export const decodeSqlResultSet = <T extends {}>(
  objs: TGenericResultSet,
  codec: t.TypeC<T>
): Array<t.TypeOf<typeof codec>> => {
  return objs.map((r: any) => {
    return decodeIOObject(r, codec);
  });
};

export const getCountries = async (
  dbConn: QueryBuilder,
  countryIds: number[]
): Promise<Country[]> => {
  const q1 = dbConn
    .select<TGenericResultSet>([
      'countries.id',
      'countries.identifier as identifier',
      'countries.name',
      'countries.currency_id',
      'currencies.identifier as currency_identifier',
      'latest_currency_rates.from_base as currency_from_base',
      'latest_currency_rates.to_base as currency_to_base'
    ])
    .from('countries')
    .join('currencies', 'countries.currency_id', '=', 'currencies.id')
    .join('latest_currency_rates', 'currencies.id', '=', 'latest_currency_rates.currency_id')
    .orderBy('name');

  const q2 = countryIds.length ? q1.whereIn('countries.id', countryIds) : q1;

  const sqlRes = await q2;
  const result = decodeSqlResultSet(sqlRes, CCountries);
  return countryIds.length ? mapResultSetToIds(countryIds, result) : result;
};

[felixmosh]

npm ls @types/bluebird

[kibertoad]

@felixmosh @loxs New version is out, hopefully fixing what was reported so far: knex@0.17.0-next4

[lorefnon]

Though I still fail to make it compile and the best thing I have so far is this:

This example will work now.

You'd want to use arrays as result type, ie. dbConn.select<TGenericResultSet> -> dbConn.select<TGenericResultSet[]>.

[felixmosh]

@felixmosh @loxs New version is out, hopefully fixing what was reported so far: knex@0.17.0-next4

WOW! it just works! KODUS for the hard work 🎊

[kibertoad]

Awesome! If no new issues are reported next week, I'll release a new stable version.

[loxs]

I can confirm that my software compiles and passes tests now.

[kibertoad]

Thank you for testing, everyone! Final version of 0.17.0 is out.