Simplify ReturningMethod type so that it avoids infinite type recursion

[laurisvan]

Fixes #2277

I am not an expert on TypeScript typings, but this problem seemed very much like the one described at a related blog article , so I tried my stab at it.

It seems to solve the problem, but I cannot guarantee if it causes some other problems. Then again, 'any' typed query builders disregard the problems anyway...

[niteshrawat1995]

@koskimas Can you have look over it please :)

[koskimas]

This is not a fix. It simply turns the result into a AnyQueryBuilder and thus removes all typing.

[koskimas]

The output is now a function of QB2 which is never assigned, so it takes the "lower bound" type AnyQueryBuilder.

[laurisvan]

@koskimas Hmm you are absolutely right. How about simply

  interface ReturningMethod {
    <QB extends AnyQueryBuilder>(
      this: QB,
      column: string | string[]
    ): QB extends NumberQueryBuilder<QB>
      ? ArrayQueryBuilder<QB>
      : QB
  }

I must say I'm getting a bit lost on these typings, so please do guide me. The TS 4.7 problem is real, and since that one is the current released version, I think it's better try to solve the problem. The snippet above solves the infinite recursion problem, but I'm not sutre it is still right (I don't claim to understand why the original checks are the way they are).

[ghost]

I've been having this problem for about a week. We started migrating our projects to node 18. Later we ran into this problem.

[vedtam]

Same here!

[laurisvan]

As per discussion, I changed the implementation so that it will return an ArrayQueryBuilder in case of NumberQueryBuilder and the normal QB in any other case. This is the solution that has worked in our cases, but as mentioned above, there might be cases (that I am unaware of) that this solution does not cover.

@koskimas Do you find that as acceptable, or how should we bend it to fix the problem?

[falkenhawk]

@laurisvan It is possible that this exposes a bug in typescript itself. Once someone posted there a reproduction for kind of similar scenario https://github.com/microsoft/TypeScript/issues/47142#issuecomment-996796794 and they reacted accordingly, preparing a fix. Maybe we could give it a shot this time too 🤔

[falkenhawk]

I've browsed typescript repo looking for hints and took one from https://github.com/microsoft/TypeScript/issues/48552#issuecomment-1093067845 to try to simplify type definitions which include recursions.

I gave it a shot and extracted the return type incl. conditional to a separate type, ...and it helped in my case. Please try it yourself @laurisvan

  interface ReturningMethod {
    <QB extends AnyQueryBuilder>(
      this: QB,
      column: string | string[]
    ): QB extends ArrayQueryBuilder<QB>
      ? ArrayQueryBuilder<QB>
      : QB extends NumberQueryBuilder<QB>
      ? ArrayQueryBuilder<QB>
      : SingleQueryBuilder<QB>;
  }

⏬

  type ReturningQueryBuilder<QB extends AnyQueryBuilder> = QB extends ArrayQueryBuilder<QB>
    ? ArrayQueryBuilder<QB>
    : QB extends NumberQueryBuilder<QB>
      ? ArrayQueryBuilder<QB>
      : SingleQueryBuilder<QB>;

  interface ReturningMethod {
    <QB extends AnyQueryBuilder>(
      this: QB,
      column: string | string[]
    ): ReturningQueryBuilder<QB>;
  }

That said, nobody shared a reproduction case either here or in linked issue https://github.com/Vincit/objection.js/issues/2277, and it would be great to have one to have something to work on instead of trying wild guesses. Simply bumping typescript to 4.7+ on objection repo and running npm run test:typings does not show any error. (as for me, I don't use returning() method at all, but I got hit by this issue in other weird places, e.g. adding modifiers to a model sometimes, but not always, triggers typescript. But i noticed when I commented out returning: ReturningMethod; from objection typings, the ts issue disappeared)

[falkenhawk]

Ok, I goofed. On more thorough check, that did not help. TS still complains TS2589: Type instantiation is excessively deep and possibly infinite.

[laurisvan]

@falkenhawk Sorry for the very late reply to this one. Did you spot some problem in my suggested solution, so that an alternate solution is still needed?

[vedtam]

Any update on this?

[laurisvan]

Any update on this?

I don't know - I have been perfectly happy with my solution and have not heard whether it is acceptable or not. The earlier comments hinted for reproduction of the case - I believe we all share the same pain that that this appears for large projects, so I'm not sure if we can package this into a regression test or such easily.

@koskimas Just share your wisdom on what should we do, and I'm perfectly happy to do so. :)

[iiq374]

Would love to see this resolved before we need to ditch objection :-/

[vedtam]

@koskimas is a great guy, unfortunately he didn't show up for some time either here or on the gitter channel. He could at least share a reason for his decision.

[laurisvan]

@koskimas is a great guy, unfortunately he didn't show up for some time either here or on the gitter channel. He could at least share a reason for his decision.

I think he has made it clear and understandable at https://github.com/Vincit/objection.js/issues/2335 - no time nor desire to maintain Objection, which I fully understand and respect.

We have been able to live by patching this change on top of an existing release using https://github.com/ds300/patch-package and that removes the immediate need to move away. Having said that, if there is a library that provides a query builder, types, and validation layer and will be maintained in foreseeable future, we are interested.

[lehni]

@kibertoad this is a crucial issue to get fixed. I think we should merge this, but can't fully judge the solution. Would love to get your opinion on it!

[lehni]

There were conflicts in the PR so I've merged this locally: c27720b1a073fcb65f9e60efa80e59b179eabfd4

Thanks for addressing this @laurisvan !