Skip to content

Improve any types for values/records in Models.d.ts #239

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

Closed
Danlock opened this issue Dec 15, 2017 · 7 comments
Closed

Improve any types for values/records in Models.d.ts #239

Danlock opened this issue Dec 15, 2017 · 7 comments

Comments

@Danlock
Copy link
Contributor

Danlock commented Dec 15, 2017

The types for functions that have to do with creation such as Model.create or Model.build have a any type on their values: parameters. Is there a reason why that can't be Partial to get some compile time verification of model attributes on such methods?

I know that this includes function names as well, but it's better than any.
I'm going to do this on my project anyway, so let me know if you would accept a PR for this.

@RobinBuschmann
Copy link
Member

RobinBuschmann commented Dec 18, 2017

Hey @Danlock. You mean this https://github.com/RobinBuschmann/sequelize-typescript/blob/master/lib/models/Model.d.ts#L308 (values?: any) for example, right? You would recommend to change this to values?: Partial<T>? Yeah, sounds good. @bilby91 Do you see any issues with this?

@chanlito
Copy link
Contributor

@RobinBuschmann currently i use Partial<Pick<T, 'id' | 'etc'>>

@Danlock
Copy link
Contributor Author

Danlock commented Dec 19, 2017

So every time you use a method like create, you have a big list of all the attributes right next to the function call? That sounds cumbersome, but I guess it's more typesafe.

Is it possible to select against subset of Model keys that are always there so that the remainder are 'legit' columns? Something like a Difference of keys between whatever Model is being passed in vs the standard Model functions and attributes we know is always there.
I'm meaning something like

type unwantedAttributes = '$get' | '$add' | 'beforeCreate' | ...
create<T,Partial<Difference<T,unwantedAttributes>> >()

If this is not possible, Partial might be the only practical solution without any knowledge of the user's attributes.

@Danlock
Copy link
Contributor Author

Danlock commented Dec 19, 2017

Ok, so I looked around for type differences and stumbled upon microsoft/TypeScript#18567.

With that I think I found the closest solution to getting the attributes for typesafety without needing to manually specify database columns.

type Diff<T extends string, U extends string> = ({ [P in T]: P } & { [P in U]: never } & { [x: string]: never })[T];
type Omit<T, K extends keyof T> = { [P in Diff<keyof T, K>]: T[P] };

type GetAttributes<T extends Model<any>> = Omit<
  T,
  | '$add'
  | '$set'
  | '$get'
  | '$count'
  | '$create'
  | '$has'
  | '$remove'
  | 'isNewRecord'
  | 'Model'
  | 'sequelize'
  | 'where'
  | 'getDataValue'
  | 'setDataValue'
  | 'get'
  | 'set'
  | 'setAttributes'
  | 'changed'
  | 'previous'
  | 'save'
  | 'reload'
  | 'validate'
  | 'update'
  | 'updateAttributes'
  | 'destroy'
  | 'restore'
  | 'increment'
  | 'decrement'
  | 'equals'
  | 'equalsOneOf'
  | 'toJSON'
>;

This will remove almost all of the already known methods from the Model instance, allowing the user's predefined columns to remain. One caveat is the sequelize defined attributes will remain as type any. For example, even if your Model just has { id: string }, using GetAttributes will result in
let attr: { id: string; createdAt: any; updatedAt: any; deletedAt: any; version: any;}

I could omit these types as well, but that would screw over people who use them, which would be a lot of people i imagine. Unless there are any objections I think ill submit a pull request sometime this week, essentially replacing the values: any in functions like create with values: GetAttributes<T>

@RobinBuschmann
Copy link
Member

Sounds good. What about Omit<T, keyof Model> and adding id, createdAt and so on manually again? Would be shorter IMO.

Omit<T, keyof Model> | {id: number; createdAt: Date; ...}

@Danlock
Copy link
Contributor Author

Danlock commented Dec 19, 2017

Sure, I can do that instead. I will be leaving those fields as type any though, since the user can use those fields for whatever they want.

@RobinBuschmann
Copy link
Member

fixed by #249 and #259

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