-
Notifications
You must be signed in to change notification settings - Fork 641
Poor TS performance in TS 4.5 leading to errors #2177
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
Comments
Splitting up the extension does let inference succeed on 4.5, but it's incredibly slow to a point where it's unusable. On 4.6-dev it immediately gives up and goes with ''any". Code example: import { Model, Page, QueryBuilder } from 'objection';
class QueryBuilderExtension<M extends Model, R = M[]> extends QueryBuilder<
M,
R
> {
ifWhere(predicate: any): this {
if (!predicate) {
return this;
} else {
return this.where('a', 1);
}
}
}
export class ExtendedQueryBuilder<
M extends Model,
R = M[]
> extends QueryBuilder<M, R> {
// These are necessary. You can just copy-paste them and change the name of the query builder class.
ArrayQueryBuilderType!: QueryBuilderExtension<M, M[]>;
SingleQueryBuilderType!: QueryBuilderExtension<M, M>;
MaybeSingleQueryBuilderType!: QueryBuilderExtension<M, M | undefined>;
NumberQueryBuilderType!: QueryBuilderExtension<M, number>;
PageQueryBuilderType!: QueryBuilderExtension<M, Page<M>>;
}
export default class Person extends Model {
id!: number;
QueryBuilderType!: ExtendedQueryBuilder<this>;
static QueryBuilder = ExtendedQueryBuilder;
static get tableName() {
return 'map';
}
}
async function getPerson() {
const k = await Person.query();
}
That's 7.8 seconds until it was able to show me the type of "k". |
You need to run typescript in strict mode. Something in typescript ~4.4 changed that broke this. This isn't a bug in objection. Objection's typings are valid typescript. They just happen to hit a corner case bug in typescript compiler itself. I don't know if it's possible to fix this for the non-strict mode. |
@koskimas I am (and always have been) running in strict mode; the tsconfig used in my example is the default. It also works in 4.4, albeit performance is a bit slow. I've checked all the TS versions from 4.4 and up and it (the minimal repro example) starts failing with version 4.5. If this is going to take a long time to fix I would suggest at least adding a warning to the "Extending the query builder" part of the documentation that it may break inference for TS >4.5; this is now the default TS version and as I've shown it is guaranteed to happen when extending the QB using the method described in the current docs. |
I've done more testing. Even with the very simplest model possible
TS performance is very slow in TS 4.5 compared to TS 4.4. Just a simple |
TS 4.4.4: TS 4.5.4: |
@JeongJuhyeon Your original repro in this issue is really snappy and never fails when I try it. The difference must be in our tsconfig files. Could you paste your's here? |
@koskimas Steps
tsconfig just for reference
If this doesn't happen for you all I can think of is an IDE difference. |
For reference, it's the same when running tsc.
|
Can anyone reproduce this in more recent TS versions than 4.5.4? I'm trying to fix this in TS but #2178 (comment) implies that this is likely fixed (and I can't reproduce it at all). |
I did a bisect using the info in this issue, and microsoft/TypeScript#47341 appears to have fixed this (which was also reported on microsoft/TypeScript#47142), so I believe this issue can be closed. |
I am unable to reproduce the problem on master with the provided example. Closing this. |
Uh oh!
There was an error while loading. Please reload this page.
Objection: 3.0.0
Knex: 0.95.14
Typescript: 4.5.4
Minimal reproducable example:
Inference fails and "k" becomes "any". On 4.5.2 it also fails most of the time, just hanging instead of inferring "any". It also fails in the 4.6 dev build.
TS Server log reveals what looks like infinite recursion going on:
The text was updated successfully, but these errors were encountered: