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

Can't get scope abstract constructors to typecheck #120

Open
danprince opened this issue Aug 4, 2017 · 17 comments
Open

Can't get scope abstract constructors to typecheck #120

danprince opened this issue Aug 4, 2017 · 17 comments

Comments

@danprince
Copy link

danprince commented Aug 4, 2017

Trying to get scopes working with these typing and running into a confusing type error. Here's a minimal example that seems to fail with the following versions:

import { Model, DataTypes } from "sequelize";
import sequelize from "../database";

class Message extends Model {}

Message.init(
  {
    private: DataTypes.BOOLEAN
  },
  {
    sequelize,
    scopes: {
      private: {
        where: {
          private: true
        }
      }
    }
  }
);

let PrivateMessages = Message.scope("private");

PrivateMessages.findAll({
  where: {}
});
// The 'this' context of type 'typeof Model' is not assignable to method's 'this' of type '(new () => Model) & typeof Model'.
//   Type 'typeof Model' is not assignable to type 'new () => Model'.
//     Cannot assign an abstract constructor type to a non-abstract constructor type.

Similarly to #112, I would expect typeof Model to be new () => Model, so I'm not sure why this intersection type fails.

@SimonSchick
Copy link
Contributor

SimonSchick commented Sep 15, 2017

#110 probably related.

Nevermind for now, different version.

@danprince
Copy link
Author

Just wanted to check in on this one again. This error is meaning that we still can't use scopes in our codebase. Are there any examples of scope code typechecking correctly or is this just something that wontfix for this combination of versions?

@SimonSchick
Copy link
Contributor

This should be resolved with my fix that was merged some time ago.

@danprince
Copy link
Author

danprince commented Nov 21, 2017

Afraid not @SimonSchick. Just trying out a few commits between your merge and HEAD.

TypeScript 2.3.4, 2.4.0 and 2.4.2:

let users = await User.scope("human").findAll({ where: { team_id: teamId } });

// The 'this' context of type 'typeof Model' is not assignable to method's 'this' of type '(new () => Model) & typeof Model'.
//   Type 'typeof Model' is not assignable to type 'new () => Model'.
//     Cannot assign an abstract constructor type to a non-abstract constructor type.

TypeScript 2.4.0:

error TS2345: Argument of type '{ where: { "slack.name": string | undefined; }; }' is not assignable to parameter of type 'FindOptions | undefined'.
  Type '{ where: { "slack.name": string | undefined; }; }' is not assignable to type 'FindOptions'.
    Types of property 'where' are incompatible.
      Type '{ "slack.name": string | undefined; }' is not assignable to type '(string | number)[] | Where | WhereAttributeHash | AndOperator | OrOperator | undefined'.
        Type '{ "slack.name": string | undefined; }' is not assignable to type 'OrOperator'.
          Property '$or' is missing in type '{ "slack.name": string | undefined; }'.

Why would the FindOptions require an $or clause in the where object? I would have expected it to match WhereAttributeHash from that union.

TypeScript 2.4.2:

team.set("slack.domain", message.event.name);

// error TS2559: Type '"slack.domain"' has no properties in common with type 'Partial<Team>'.

@SimonSchick
Copy link
Contributor

TS attempts to match your type to all listed types and will end up telling you only about one mismatch, the interesting bit is here: (string | number)[] | Where | WhereAttributeHash | AndOperator | OrOperator | undefined.

@danprince
Copy link
Author

@SimonSchick I'd understand that if it was an intersection type, but why would it report an error when one of the types in the union does match?

@SimonSchick
Copy link
Contributor

I think it doesn't like the undefined type union, you probably have strict null checks enabled...

You might want to manually assert that the value is not undefined via if or suffix the index with a ! eg. where: { team_id: teamId! } } as only null is allowed in the WhereAttributeHash type.

@danprince
Copy link
Author

I do have strict null checks enabled, but the error is thrown regardless of which method I try to invoke on User.scope().

image

I've just turned them off and get the exact same compile errors.

@SimonSchick
Copy link
Contributor

Wait, is User abstract in your code?

@danprince
Copy link
Author

danprince commented Nov 21, 2017

@SimonSchick
Copy link
Contributor

Ooooh, I see what you mean now, I ran into the same thing.

So here is what you can do:

Add a override for all scopes you have to your model.

Eg.

public static scope(options?: ScopeStringType | ScopeStringType[] | ScopeOptions | WhereAttributeHash): typeof YourModel {
    return <typeof YourModel> super.scope(options);
  }

Also your scope is wrong, should probably be:

scopes: {
  human: {
    where: {
      slack_id: { $ne: 'USLACKBOT' }
    },
  },
},

@danprince
Copy link
Author

Thanks for helping out with this! Using that override would mean adding that code to all of our models though right?

@SimonSchick
Copy link
Contributor

@danprince all models you call .scope static on, yes.
The problem here is that the base .scope has the return signature typeof Model whereas Model is abstract.

@danprince
Copy link
Author

Ah. I see, but then isn't this broken for everyone?

@SimonSchick
Copy link
Contributor

SimonSchick commented Nov 21, 2017

Yes and no, I assume @felixfbecker intention was for this to be overridden on a per model basis.

An alternative would be to change to call signature of .scope to something more generic such as:

export interface ModelConstructor {
  new (...args: []): Model;
}
...
abstract class Model {
  ...
  static scope<M extends ModelConstructor>(this: M, options?: string | Array<string> | ScopeOptions | WhereAttributeHash): M;
}

Thoughts @felixfbecker ?

@felixfbecker
Copy link
Collaborator

Ideally scope should return typeof this or typeof this['constructor'] but I don't think that's possible. It might be possible to use generics as a workaround, although not pretty.

@SimonSchick
Copy link
Contributor

The generics workaround I posted works pretty well.

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

No branches or pull requests

3 participants