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

Generics for the model attributes? #125

Open
rokoroku opened this issue Sep 5, 2017 · 6 comments
Open

Generics for the model attributes? #125

rokoroku opened this issue Sep 5, 2017 · 6 comments

Comments

@rokoroku
Copy link
Contributor

rokoroku commented Sep 5, 2017

Why don't we have generic for the model attributes?
e.g.

interface TypedModelAttributes {
  id: number;
  optionalData?: object;
}

class TypedModel extends Model<TypedModelAttributes> {
  ...
}

// usage
TypedModel.build({ ... }) // will be type-checked by "TypedModelAttributes"
TypedModel.findOne({ where: { ... }) // will be type-checked by "keyof TypedModelAttributes"
...

It is more accurate and will bring a lot of conveniences than current <K extends keyof this> way.

@felixfbecker
Copy link
Collaborator

Why do you think is it more accurate than this?

class TypedModel extends Model {
  public id: number;
  public optionalData?: object;
}

The attributes are attributes of the class, so imo they should be declared there.
What conveniences will it bring exactly?

@rokoroku
Copy link
Contributor Author

rokoroku commented Sep 5, 2017

when we define custom model method

class TypedModel extends Model {
  public id: number;
  public optionalData?: object;
  aMethod() {
    ...
  }
  anotherMethod() {
    ...
  }
}

aMethod and anotherMethod can be treated as model's attributes in the <K extends keyof this> scenario.

Furthermore, the model prototype's own method (such as get, set, toJSON, ... ) also treated as their attributes, so it makes the intellisense (quick suggestions) bulky with unnecessary values.

@SimonSchick
Copy link
Contributor

Making the attributes a model would also properly restrict attributes: [...], and allow .get() to return correct objects.

@felixfbecker
Copy link
Collaborator

I don't think it's that big of an issue that you can also pass the methods. I don't think that would cause any error in real-world usage. On the other hand, having to duplicate the attribute declaration in both the class and an interface is cumbersome.

@rokoroku
Copy link
Contributor Author

Though writing same attribute declaration in class and interface is bit cumbersome, it's not a requirement. We can also write a model without generic thanks to the TypeScripts' default type parameters.

Like @SimonSchick said, generic attributes will provide more help when we write where or attributes clause, also allowing any sequelize.fn type too.

If you don't mind, I'll try to make a suggestion PR from below scratch.

type WhereAttributeHash<TAttributes = any> = {
  [K in keyof TAttributes]?: TAttributes[K] | WhereValue<TAttributes[K]>;
};

type WhereValue<TAttributes = any> =
  TAttributes
  | null
  | WhereOperators<TAttributes>
  | WhereAttributeHash<TAttributes> // for JSON columns
  | Col<TAttributes> // reference another column
  | OrOperator<TAttributes>
  | AndOperator<TAttributes>
  | WhereGeometryOptions<TAttributes>
  | Array<TAttributes | WhereAttributeHash<TAttributes>>; // implicit $or

type WhereOptions<T = any> = WhereAttributeHash<T> | null | T | T[];

@SimonSchick
Copy link
Contributor

SimonSchick commented Sep 24, 2017

One way to achieve this without massive duplication would be:

type Constructor<T> = new (...args: any[]) => T;

function mixin<TI, T extends typeof Mode l= typeof Model>(cls: T): T & Constructor<TI> {
  return <T & Constructor<TI>> <any> cls;
}

export interface RoleProperties {
  userName: string;
}

// tslint:disable-next-line:max-classes-per-file
export class Role extends mixin<RoleProperties>(Model) {
...
}

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