Type inferred is wrong when using model constructor

[lorenzopicoli]

After version 3.0.3, more specifically after this PR: #2399 I've been encountering a breaking change to types.

I define my own query builder like this:

export class BaseModelWithOnlyTimestamp
  extends Model
{
  // Both of these are needed.
  public QueryBuilderType!: MyQueryBuilder<this>
  public static QueryBuilder = MyQueryBuilder

  @Hook('beforeInsert', () => new Date().toISOString())
  public createdAt?: string

  @Hook('beforeUpdate', () => new Date().toISOString())
  public updatedAt?: string

   constructor(props?: { updatedAt?: string; createdAt?: string }) {
     super()
     this.updatedAt = props?.updatedAt
     this.createdAt = props?.createdAt
   }
}

Then I call the following function:

      let qb = MyModel.relatedQuery(related).withGraphJoined(...)

Before the PR mentioned, the type of qb would be MyQueryBuilder<MyModel> which would be correct.

After the update the type becomes QueryBuilder<Objection.Model>

If I change the code to have a constructor that doens't take any arguments then the code works again as expected. Reverting the changes from the PR also results in correct types

[max-kahnt-keylight]

I've been proposing the change that apparently creates this issue. I am happy to help out for a bit to see if we find the reason for it and can maybe fix or mitigate it.

I failed to reproduce the issue with my local setup, i.e. everything works on my end when I run the code I believe the example implies. This might happen due to insufficient setup of the customized query builder (see https://vincit.github.io/objection.js/recipes/custom-query-builder.html#extending-the-query-builder-in-typescript) or maybe due to version differences in typescript. I am currently using typescript 4.8.4.

@lorenzopicoli Can you provide a minimal example including your TenantQueryBuilder setup and specifying your typescript version?
I will also make some random guesses what to try in order to narrow down the source of the problem. I cannot justify why any of these would be related, though, and, of course, ultimately I am not recommending to get rid of the respective functionality - just trying to figure out what to look at next :)

• No decorators
• No explicit constructor
• Use interface definition instead of type in https://github.com/Vincit/objection.js/pull/2399/files

Hope any of these will help to push this issue further. 🤞🏼

[lorenzopicoli]

Thank you for the quick reply @max-kahnt-keylight.

I've managed to create reproducible repo: https://github.com/lorenzopicoli/objection-reproduce.

You can run npm start to try to compile the code and see the error

You'll see that I included a few things that I found out while testing.

1. You'll find 3 examples of constructors on the base class, you can comment and uncomment them to see some working and broke examples. Notice that having no constructor at all works.
2. I believe that the BaseClass is import to reproduce this. Defining the query builder types in the animal class might fix it? I'm not sure
3. I stumbled upon a ts configuration file that actually compiles the example just fine, but I'm not sure why. I've included it in the repo
4. Notice that manually commenting out your changes in the objection definition files allows any of the examples to work

Let me know if I can help with anything else, this looks like more and more like an edge case to me, but I can't figure out what exactly causes typescript to be confused.

[max-kahnt-keylight]

@lorenzopicoli Thank you for the reproduction example. It helped a lot.

Some quick learnings to share: I believe the issue only happens with tsconfig.json's strictFunctionTypes: true enabled.
Notably, my IDE then agrees that the typing for relatedQuery should be deduced from

objection.js/typings/objection/index.d.ts

Line 1577 in 8e8ff2f

static relatedQuery<RM extends Model>(

and not from 6 lines above (presumably because the constructor type cannot be inferred). This is where the non-satisfying resulting query builder's type should originate from.

#702 sounds strongly related. It sounds like the decision back then was to not support this flag. (Edit) Maybe in the meantime there are ways to fix the constructor typing, other related objection typings (like the relatedQuery function type whose overload signatures I cannot quite justify), or configure (a newer version of) typescript in an appropriate way or maybe get rid of ModelClass<> now in favor of proper static interface deduction? (Edit)
I'll attempt to investigate this more, but I figured I should share this early since I might get stuck given the discussion in my findings.

Edit: I can definitely see that a revert of the Constructor typing changes is reasonable given these difficulties.

[max-kahnt-keylight]

@lorenzopicoli This fix seems to build against the existing objection test typings, your issue example, and also my codebase :D. There are probably more places in objection's current typings this should be applied to for this to be ready for review.
It feels like quite an ugly workaround but given the discussions I found so far, IMO there is some justification to address this issue with such a thing. For the typings at objection, it seems to match the non-fallback relatedQuery() function type overload and hence properly deduces the required model (and associated query builder).

We should maybe add this use-case to the objection test typings as well such that the requirements become a bit more stable and well-defined, since some gaps will remain for sure.

@lehni How would you feel about such an approach? Do you think it would be mergable?

[lehni]

@max-kahnt-keylight thank you for working on this! I have to admit that my knowledge of TS is very limited, and I am not actually relying on typings so much in my daily work, so I can't fully judge this unfortunately. Perhaps @falkenhawk would be willing to have a look and chime in?

[lehni]

I'd say let's roll out a version with 23b8ac1 reverted and take more time to figure out the proper solution, and make sure it won't cause other new issues.

[lorenzopicoli]

Thank you again @max-kahnt-keylight for taking the time to fix this.

Just wanted to confirm that on my project changing strictFunctionTypes to false fixes the issue and applying your branch locally also fixes the issue.