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

Compilation breaks with TypeScript 3.9 #1750

Closed
oranoran opened this issue May 13, 2020 · 15 comments
Closed

Compilation breaks with TypeScript 3.9 #1750

oranoran opened this issue May 13, 2020 · 15 comments

Comments

@oranoran
Copy link

Upgraded a project from Typescript 3.8.2 to TypeScript 3.9.2.
The project is using the latest Objection (2.1.3).

After the TS upgrade, the project fails to compile with compilation errors related to Objection types.

I suspect this is related to "Stricter Checks on Intersections and Optional Properties" as described here.

node_modules/objection/typings/objection/index.d.ts:268:43 - error TS2344: Type 'Model & T' does not satisfy the constraint 'Model'.
  The types of 'QueryBuilderType.findById(...).update(...).page(...).insertGraph' are incompatible between these types.
    Type 'InsertGraphMethod<Model & T>' is not assignable to type 'InsertGraphMethod<Model>'.
      Types of parameters 'graph' and 'graph' are incompatible.
        Type 'PartialModelGraph<Model, Model & GraphParameters>' is not assignable to type 'PartialModelGraph<Model & T, Model & T & GraphParameters>'.

268     ? SingleQueryBuilder<QueryBuilderType<T>>
                                              ~

node_modules/objection/typings/objection/index.d.ts:271:26 - error TS2344: Type 'Model & I' does not satisfy the constraint 'Model'.
  The types of 'QueryBuilderType.findById(...).update(...).page(...).insertGraph' are incompatible between these types.
    Type 'InsertGraphMethod<Model & I>' is not assignable to type 'InsertGraphMethod<Model>'.
      Types of parameters 'graph' and 'graph' are incompatible.
        Type 'PartialModelGraph<Model, Model & GraphParameters>' is not assignable to type 'PartialModelGraph<Model & I, Model & I & GraphParameters>'.

271       ? QueryBuilderType<I>
                             ~

node_modules/objection/typings/objection/index.d.ts:281:24 - error TS2344: Type 'Model & T' does not satisfy the constraint 'Model'.
  The types of 'QueryBuilderType.findById(...).update(...).page(...).insertGraph' are incompatible between these types.
    Type 'InsertGraphMethod<Model & T>' is not assignable to type 'InsertGraphMethod<Model>'.
      Types of parameters 'graph' and 'graph' are incompatible.
        Type 'PartialModelGraph<Model, Model & GraphParameters>' is not assignable to type 'PartialModelGraph<Model & T, Model & T & GraphParameters>'.

281     ? QueryBuilderType<T>
                           ~

node_modules/objection/typings/objection/index.d.ts:284:26 - error TS2344: Type 'Model & I' does not satisfy the constraint 'Model'.
  The types of 'QueryBuilderType.findById(...).update(...).page(...).insertGraph' are incompatible between these types.
    Type 'InsertGraphMethod<Model & I>' is not assignable to type 'InsertGraphMethod<Model>'.
      Types of parameters 'graph' and 'graph' are incompatible.
        Type 'PartialModelGraph<Model, Model & GraphParameters>' is not assignable to type 'PartialModelGraph<Model & I, Model & I & GraphParameters>'.

284       ? QueryBuilderType<I>```
@koskimas
Copy link
Collaborator

koskimas commented May 16, 2020

This is a pretty impossible situation. The thing that's breaking is actually a fix for this typescript bug. That bug has been fixed in the current typescript version, but if I roll back the objection fix that works around that bug, the typings will be broken for everyone still using typescript <= 3.8.

Somebody will suggest a major version bump, but that's not going to happen. Those are reserved for bigger changes.

The only thing we can do is to revert the fix to make typescript 3.9 work, and then we'll get ten of these issues about things being broken with typescript <= 3.8.

@koskimas
Copy link
Collaborator

See this coment microsoft/TypeScript#33460 (comment)

@koskimas
Copy link
Collaborator

This is the fix in objection 46c3105.

Actually, could somebody try to revert the above fix to see if the problem goes away? It fixed the problem in my small test, but I'd like to be sure it's the only thing broken.

@francoisromain
Copy link

Hello @koskimas

I updated to typescript 3.9.2 and had the above error.
If I revert the fix in objection's index.d.ts, I still have an error:

node_modules/objection/typings/objection/index.d.ts:268:43 - error TS2344: Type 'Model & T' does not satisfy the constraint 'Model'.
  The types of 'QueryBuilderType.findById(...).update(...).page(...).insertGraph' are incompatible between these types.
    Type 'InsertGraphMethod<Model & T>' is not assignable to type 'InsertGraphMethod<Model>'.
      Types of parameters 'graph' and 'graph' are incompatible.
        Type 'PartialModelGraph<Model>' is not assignable to type 'PartialModelGraph<Model & T>'.
          Type 'PartialModelGraph<Model>' is not assignable to type '{ [K in { [K in keyof (Model & T)]: (Model & T)[K] extends Function ? never : K; }["toJSON" | "$id" | "QueryBuilderType" | "$relatedQuery" | "$query" | ... 29 more ... | keyof T]]?: (Exclude<...> extends Model ? PartialModelGraph<...> : Exclude<...> extends (infer I)[] ? I extends Model ? PartialModelGraph<...>[] : ...'.

268     ? SingleQueryBuilder<QueryBuilderType<T>>
                                              ~

node_modules/objection/typings/objection/index.d.ts:271:26 - error TS2344: Type 'Model & I' does not satisfy the constraint 'Model'.
  The types of 'QueryBuilderType.findById(...).update(...).page(...).insertGraph' are incompatible between these types.
    Type 'InsertGraphMethod<Model & I>' is not assignable to type 'InsertGraphMethod<Model>'.
      Types of parameters 'graph' and 'graph' are incompatible.
        Type 'PartialModelGraph<Model>' is not assignable to type 'PartialModelGraph<Model & I>'.
          Type 'PartialModelGraph<Model>' is not assignable to type '{ [K in { [K in keyof (Model & I)]: (Model & I)[K] extends Function ? never : K; }["toJSON" | "$id" | "QueryBuilderType" | "$relatedQuery" | "$query" | ... 29 more ... | keyof I]]?: (Exclude<...> extends Model ? PartialModelGraph<...> : Exclude<...> extends (infer I)[] ? I extends Model ? PartialModelGraph<...>[] : ...'.

271       ? QueryBuilderType<I>
                             ~

node_modules/objection/typings/objection/index.d.ts:281:24 - error TS2344: Type 'Model & T' does not satisfy the constraint 'Model'.
  The types of 'QueryBuilderType.findById(...).update(...).page(...).insertGraph' are incompatible between these types.
    Type 'InsertGraphMethod<Model & T>' is not assignable to type 'InsertGraphMethod<Model>'.
      Types of parameters 'graph' and 'graph' are incompatible.
        Type 'PartialModelGraph<Model>' is not assignable to type 'PartialModelGraph<Model & T>'.
          Type 'PartialModelGraph<Model>' is not assignable to type '{ [K in { [K in keyof (Model & T)]: (Model & T)[K] extends Function ? never : K; }["toJSON" | "$id" | "QueryBuilderType" | "$relatedQuery" | "$query" | ... 29 more ... | keyof T]]?: (Exclude<...> extends Model ? PartialModelGraph<...> : Exclude<...> extends (infer I)[] ? I extends Model ? PartialModelGraph<...>[] : ...'.

281     ? QueryBuilderType<T>
                           ~

node_modules/objection/typings/objection/index.d.ts:284:26 - error TS2344: Type 'Model & I' does not satisfy the constraint 'Model'.
  The types of 'QueryBuilderType.findById(...).update(...).page(...).insertGraph' are incompatible between these types.
    Type 'InsertGraphMethod<Model & I>' is not assignable to type 'InsertGraphMethod<Model>'.
      Types of parameters 'graph' and 'graph' are incompatible.
        Type 'PartialModelGraph<Model>' is not assignable to type 'PartialModelGraph<Model & I>'.
          Type 'PartialModelGraph<Model>' is not assignable to type '{ [K in { [K in keyof (Model & I)]: (Model & I)[K] extends Function ? never : K; }["toJSON" | "$id" | "QueryBuilderType" | "$relatedQuery" | "$query" | ... 29 more ... | keyof I]]?: (Exclude<...> extends Model ? PartialModelGraph<...> : Exclude<...> extends (infer I)[] ? I extends Model ? PartialModelGraph<...>[] : ...'.

284       ? QueryBuilderType<I>
                             ~


Found 4 errors.

@koskimas
Copy link
Collaborator

@francoisromain Thanks. I'm able to reproduce this now too.

I'm baffled. I have no idea how to fix this. There's not a single A & B type in objection types anywhere (after changing the GraphParameters thing) and I still get the error. I don't think I'll be able to solve this one. I really need help from people that are more skilled in typescript.

@koskimas
Copy link
Collaborator

Haha, I may have found a solution by accident. Could you try the typescript-39-support branch to see if you get any problems? I'd especially like to know that we don't lose any type safety (things don't turn into any because of this).

@francoisromain
Copy link

@koskimas the modification in the typescript-39-support branch fixed the compilation with ts 3.9.2. Cool! I am not sure how to check this part:

I'd especially like to know that we don't lose any type safety (things don't turn into any because of this).

@koskimas
Copy link
Collaborator

koskimas commented May 19, 2020

That's a bit tricky as the any type spreads like cancer. Even more so if you don't have the noExplicitAny config enabled. I went through the type tests (by checking the actual type of the expressions) and I couldn't find any problems. I think I'm going to go ahead and release the fix tomorrow. I need to add some real tests for the typings using something like dtslint.

@mastermatt
Copy link
Contributor

dtslint is great, I use it a lot, but be warned that TSLint is deprecated.

@kibertoad
Copy link
Collaborator

See microsoft/dtslint#284

@Kinrany
Copy link

Kinrany commented May 22, 2020

You might be able to maintain backwards compatibility by detecting the TypeScript version and providing different types based on that.

Using the example from the TS 3.9 announcement: (playground)

interface A {
    a: number; // notice this is 'number'
}

interface B {
    b: string;
}

interface C {
    a?: boolean; // notice this is 'boolean'
    b: string;
}

// this type will be different depending on the compiler version
type Result = A & B extends C ? '< 3.9' : '>= 3.9';

@koskimas
Copy link
Collaborator

There's no backwards compatibility issues with the fix.

@phr3nzy
Copy link

phr3nzy commented May 25, 2020

You can set:

"strict": false,
"strictFunctionTypes": false,
"alwaysStrict": false,

in your tsconfig.json as a temp workaround.

@Kinrany
Copy link

Kinrany commented May 25, 2020

Disabling "strict" turns TS into a glorified linter.

A better workaround is to import the git branch with the fix. Or fork the repo at the last released version and cherry-pick the fix.

@koskimas
Copy link
Collaborator

koskimas commented May 26, 2020

The fix is now released. No need for any workarounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants