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

Make function types generic in type definitions #3168

Merged
merged 2 commits into from May 4, 2019

Conversation

Projects
None yet
6 participants
@lorefnon
Copy link
Contributor

commented Apr 27, 2019

No description provided.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Apr 27, 2019

Impressive! What makes it WIP?

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

@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

This comment has been minimized.

Copy link

commented Apr 28, 2019

@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 lorefnon force-pushed the lorefnon:generic-type-defs branch 3 times, most recently from e71cf3a to d0ab03c Apr 30, 2019

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@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:

knex-select-autocomplete

Auto completion of column name in where:

knex-where-auto-complete

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

knex-where-value-type-autocomplete

Type safety in update argument:

update-partial-type-error

@lorefnon lorefnon force-pushed the lorefnon:generic-type-defs branch 3 times, most recently from 0643a11 to 790f56c May 1, 2019

@lorefnon lorefnon changed the title [WIP] Make function types generic in type definitions Make function types generic in type definitions May 1, 2019

@lorefnon lorefnon marked this pull request as ready for review May 1, 2019

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

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

@pvider

This comment has been minimized.

Copy link

commented May 1, 2019

@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 lorefnon force-pushed the lorefnon:generic-type-defs branch 2 times, most recently from cc109c4 to ed96330 May 2, 2019

@lorefnon lorefnon force-pushed the lorefnon:generic-type-defs branch from ed96330 to 7051d7c May 2, 2019

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

@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 kibertoad merged commit 6a4fecf into tgriesser:master May 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 85.325%
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 4, 2019

Thanks a lot!

@Xiphe

This comment has been minimized.

Copy link

commented May 13, 2019

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

This comment has been minimized.

Copy link

commented May 13, 2019

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.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Ok alright. Sounds good to me.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

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

  1. usage of .first() still return a array
    image
  2. there is an error with ChainableInterface,
    image
  3. .insert() return the wrong value, it suppose to return an array with one element (the insert id).
    image
  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(),
    image

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

@loxs

This comment has been minimized.

Copy link

commented May 17, 2019

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

This comment has been minimized.

Copy link

commented May 17, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

@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

This comment has been minimized.

Copy link

commented May 18, 2019

@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

This comment has been minimized.

Copy link

commented May 18, 2019

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

@loxs

This comment has been minimized.

Copy link

commented May 18, 2019

One bug report:

Screenshot 2019-05-18 at 9 49 11

This should probably be string instead of string[]

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2019

Good catch. Will fix asap, sorry

@loxs

This comment has been minimized.

Copy link

commented May 18, 2019

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):

Screenshot 2019-05-18 at 10 07 19

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

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

@loxs

This comment has been minimized.

Copy link

commented May 18, 2019

@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:

Screenshot 2019-05-18 at 14 43 46

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

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

npm ls @types/bluebird

image

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2019

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

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2019

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

@loxs

This comment has been minimized.

Copy link

commented May 19, 2019

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.