Hey @mceachen. Sami suggested that I open a new issue for this, though it's a continuation of the sorts of problems in #717. Boy did I learn a lot studying your typings!
I'm sharing from my fork before making any PRs:
These new typings tests include a number of errors mostly related to not passing the result value along the chain of query builder calls. The only tests in here that do work are those testing throwIfNotFound(), which I've added because they helped me test my mods.
These revised typings that resolve the above new tests. The type parameterization of QueryBuilder in a few of the Typescript tests also had to change.
I made the following sorts of changes to the typings, going from memory:
Query builders now pass the expected result type along the chain. The declaration is now QueryBuilder<QM, RM, RV>, where QM = queried model, RM = candidate result model or model array, and RV = actual result value. QM is different from RM because RM can be either QM or QM[]. RV can be QM, QM[], QM | undefined, and number.
The query builder interfaces went through quite an evolution but they eventually came back to something like you originally had. The names have changed to identify the functionality they offer, so that may be understood without having to look up the interface. However, I recognize there is a drawback to not naming things according to their SQL correlates (e.g. insert, update, delete, etc.), because odds are that the typings will evolve according to their correlates. I can deal with either approach but wanted to present an alternative. I can definitively say that the approach I'm illustrating would have helped me a lot scanning through the typings.
Generic parameters all have suggestive names. The T that originally appeared throughout threw me for a loop because I was confused about whether that was meant to represent a model or a QueryBuilder or a transaction or what. It took examining the typings at length to understand what it was intended to mean.
Page query builders now support the runAfter() method.
The following remains to be done. I've given them a preliminary look and have questions about both. It would be helpful to collaborate.
Possibly accommodate any QueryBuilder result type returned by callbacks, per Sami's comment. It may be that just the eager modify/filter callbacks need a type any.
Figure out how to support custom query builders in Typescript. I'm stuck at Model.query() needing to return a custom QueryBuilder type. When I parameterize Model and use that parameter in this static method, I get an error saying, "Static members cannot reference class type parameters." It would be a pain to have to parameterize each call to Model.query(). Recall also that I've demoed a quick-fix to the existing typings that gets custom query builders working.
Perhaps make additional changes to help make the typings clearer, such as expanding interface definitions of methods into the methods themselves where practical (there are a number of one-offs, for example), using a type naming convention to help distinguish knex methods from objection methods, organizing the document into sections with headers, and comments that might help a reader who is browsing for type information understand what they're looking at.
I really had a hard time improving on what you've already done. The only significant change I could come up with was finding a way to make Sami's suggestion work, although I ended up doing something slightly different (QueryBuilder<QM, RM, RV> vs QueryBuilder<QM, SingleModel, ArrayModel>).
mceachen
commented
6 years ago
Hey @mceachen. Sami suggested that I open a new issue for this, though it's a continuation of the sorts of problems in #717. Boy did I learn a lot studying your typings!
I'm sharing from my fork before making any PRs:
throwIfNotFound(), which I've added because they helped me test my mods.I made the following sorts of changes to the typings, going from memory:
QueryBuilder<QM, RM, RV>, whereQM= queried model,RM= candidate result model or model array, andRV= actual result value.QMis different fromRMbecauseRMcan be eitherQMorQM[].RVcan beQM,QM[],QM | undefined, andnumber.Tthat originally appeared throughout threw me for a loop because I was confused about whether that was meant to represent a model or a QueryBuilder or a transaction or what. It took examining the typings at length to understand what it was intended to mean.runAfter()method.The following remains to be done. I've given them a preliminary look and have questions about both. It would be helpful to collaborate.
any.Model.query()needing to return a custom QueryBuilder type. When I parameterize Model and use that parameter in this static method, I get an error saying, "Static members cannot reference class type parameters." It would be a pain to have to parameterize each call toModel.query(). Recall also that I've demoed a quick-fix to the existing typings that gets custom query builders working.I really had a hard time improving on what you've already done. The only significant change I could come up with was finding a way to make Sami's suggestion work, although I ended up doing something slightly different (
QueryBuilder<QM, RM, RV>vsQueryBuilder<QM, SingleModel, ArrayModel>).Thanks you for you continued patience with me!