feat(types): added optional stricter typing for Model attributes #12405

hsource · 2020-06-22T10:19:56Z

Pull Request check-list

Please make sure to review and check all of these items:

Does npm run test or npm run test-DIALECT pass with this change (including linting)? **Also run npx tsc --project types/test/ and npx tsc --project types/
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Have you added new tests to prevent regressions?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Did you update the typescript typings accordingly (if applicable)?
Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

This code allows users to have typechecking for model attributes in all relevant functions in the Model class, as well as some functions outside of the Model class. This was inspired by the Sequelize v4 DefinitelyTyped typings, but is made to be almost fully backwards compatible for users of the current typings.

Specific things I tried to ensure:

Works with sequelize.define as well as new Model formats of building models
Use the attributes as much as possible
Allows for omitting the types - everything still works fine without them

Here are the things I changed:

Added 2 dummy variables (_attributes and _creationAttributes) to the Model class so that the static functions can be typed correctly
- Used the dummy types to get the attributes type in static functions in model.d.ts
- Before this, I'd tried doing the following, but Typescript fails to infer the TAttributes automatically

  public static findOne<M extends Model<TAttributes>, TAttributes>(
    this: { new(): M },
    options: NonNullFindOptions<TAttributes>
  ): Promise<M>;

Used the dummy types to get the attributes type in static functions in model.d.ts

Retyped some functions outside of model.d.ts to use the new typings, and made others just fallback to just using any for the generic arguments
Added tests and updated existing tests in types/tests

Testing

In addition to adding a few tests and modifying the tests for User.ts and define.ts to use the new typings, I also have this running on a codebase with ~20k LOC for https://wanderlog.com - everything typechecks correctly, and it's an improvement from our v4 typings.

Questions for reviewers

Currently, some of the Option interfaces like FindOptions now has a required generic parameter. Should I make this parameter also optional to make it fully backwards-compatible?
- This means users who had their own function like doFancyFind(option: FindOptions) won't need to change it to doFancyFind(option: FindOptions<any>) with this change
Is it OK to just bundle an Optional type in the typings in index.d.ts? It's very useful for constructing the optional types and can help

codecov · 2020-06-22T10:28:38Z

Codecov Report

Merging #12405 into master will increase coverage by 5.96%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12405      +/-   ##
==========================================
+ Coverage   90.28%   96.25%   +5.96%     
==========================================
  Files          92       95       +3     
  Lines        8968     9308     +340     
==========================================
+ Hits         8097     8959     +862     
+ Misses        871      349     -522

Impacted Files	Coverage Δ
lib/sequelize.js	`92.53% <0.00%> (-2.65%)`	⬇️
lib/dialects/mssql/query-interface.js	`100.00% <0.00%> (ø)`
lib/dialects/mssql/index.js	`100.00% <0.00%> (ø)`
lib/dialects/mssql/connection-manager.js	`87.17% <0.00%> (ø)`
lib/model.js	`96.62% <0.00%> (+0.06%)`	⬆️
lib/dialects/abstract/query-generator.js	`97.39% <0.00%> (+1.26%)`	⬆️
...dialects/abstract/query-generator/helpers/quote.js	`100.00% <0.00%> (+6.66%)`	⬆️
lib/dialects/mssql/data-types.js	`100.00% <0.00%> (+69.87%)`	⬆️
lib/dialects/mssql/async-queue.js	`100.00% <0.00%> (+76.47%)`	⬆️
lib/dialects/mssql/query.js	`96.66% <0.00%> (+91.66%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e33d2bd...755836f. Read the comment docs.

hsource

A few comments about why I changed specific things -

hsource · 2020-06-22T10:30:52Z

types/test/errors.ts

@@ -5,7 +5,7 @@ import { OptimisticLockError } from '../lib/errors';

 async function test() {
  try {
-    await User.create({ username: 'john_doe' });


I made firstName required in the attributes I defined, so I updated this code to match

hsource · 2020-06-22T10:45:30Z

types/lib/model.d.ts

@@ -685,28 +683,28 @@ export interface Hookable {
 /**
 * Options for Model.findOrCreate method
 */
-export interface FindOrCreateOptions extends Filterable, Logging, Transactionable {
+export interface FindOrCreateOptions<TAttributes, TCreationAttributes> extends FindOptions<TAttributes> {


I made this code just extend FindOptions as other options (like attributes) actually are accepted too, per the code inside model.js which just directly does a findOne with the options.

hsource · 2020-06-22T23:06:33Z

I made some fixes to the documentation and this is now ready for review!

Edit: There's still some problems getting this to work both for db.define models and class-based defined models.
Edit 2: I switched the order of types in ModelCtor and that fixed it! Added a comment and bundled in a few other fixes

sushantdhiman · 2020-06-23T04:42:23Z

Currently, some of the Option interfaces like FindOptions now has a required generic parameter. Should I make this parameter also optional to make it fully backwards-compatible?

This means users who had their own function like doFancyFind(option: FindOptions) won't need to change it to doFancyFind(option: FindOptions<any>) with this change

Yes make those generic parameters optional, we should avoid breaking change if we can.

Is it OK to just bundle an Optional type in the typings in index.d.ts? It's very useful for constructing the optional types and can help

No problem with exporting this helper type

sushantdhiman

LGTM

sushantdhiman · 2020-06-23T04:53:27Z

types/lib/model.d.ts

-    values?: object,
-    options?: CreateOptions
+    this: ModelStatic<M>,
+    values?: M['_creationAttributes'],


It is possible to create record with id i.e. primary key. Distinction between CreationAttributes and Attributes from user point of view make sense, but internally all attributes are accepted.

We can also think about accepting TAttributes generic which contains all attributes, then create a dynamic type for "CreationAttributes" by making primary key attribute optional in that type

This is an interesting idea! The problem is - how do we figure out what's the primary key? We'd probably need users to provide it in a new generic (maybe TPrimaryKey) but then that's just one more thing to provide. And then, there are still fields with defaultValue which users can also omit, and other cases we'd need to handle.

I think TCreationAttributes is a pretty flexible solution that solves for both primary keys and other columns with default values that can be omitted. It's also been used for the previous sequelize v4 typings, so would be familiar to those users.

Right, there is no good way to infer primary key in current Typescript schema. Actual attributes are passed to init or define, which contains this info.

Yeah, I think defining it in init is especially problematic, so I recommend just leaving it as-is.

hsource · 2020-06-23T09:15:22Z

Currently, some of the Option interfaces like FindOptions now has a required generic parameter. Should I make this parameter also optional to make it fully backwards-compatible?

This means users who had their own function like doFancyFind(option: FindOptions) won't need to change it to doFancyFind(option: FindOptions<any>) with this change

Yes make those generic parameters optional, we should avoid breaking change if we can.

I just pushed a new change that made all types optional. I reverted some of the changes adding explicit <any> types to the types/test dir to test it. Everything seems to typecheck correctly!

Note that I did leave in the explicit <any> types in other definition files like sequelize.d.ts. I think it makes it easier for future readers that these types being used there can accept more specific types.

sushantdhiman · 2020-06-26T08:34:30Z

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

zaggino · 2020-06-26T11:51:56Z

Just a question, would it be possible somehow to drop repeating of this?

hsource · 2020-06-27T01:13:49Z

Just a question, would it be possible somehow to drop repeating of this?

Ideally, we'd be able to extend multiple classes and be able to do something like this below. But Typescript does not have multiple inheritance:

class UserAttributes {
  public id!: number;
  public username!: string;
  public firstName!: string;
  public lastName!: string;
  public groupId!: number;
}

export class User extends Model<UserAttributes, UserCreationAttributes>, UserAttributes {
  public static associations: {
    group: BelongsTo<User, UserGroup>;
  };

  public createdAt!: Date;
  public updatedAt!: Date;

  // mixins for association (optional)
  public group?: UserGroup;
  public getGroup!: BelongsToGetAssociationMixin<UserGroup>;
  public setGroup!: BelongsToSetAssociationMixin<UserGroup, number>;
  public createGroup!: BelongsToCreateAssociationMixin<UserGroup>;
}

This isn't possible because Typescript doesn't support multiple inheritance, but you might be able to achieve something similar with Mixins - https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-2.html#putting-all-of-the-above-rules-together-in-an-example

Try it out! If you find a good solution, it'd be cool to post below and maybe even to incorporate it into the typings for the library.

knoid · 2020-08-02T03:55:14Z

I can't find a way to make this work when doing a create with include. e.g.:

class User extends Model<UserAttributes, UserCreationAttributes> {...}
class Project extends Model<ProjectAttributes, ProjectCreationAttributes> {...}
Project.hasMany(User);
Project.create({
  users: [{ name: 'project owner' }],
}, { include: [User] });

I get the error: "Object literal may only specify known properties, and 'users' does not exist in type ProjectCreationAttributes".

I'd add users into ProjectCreationAttributes but that's just a Optional<ProjectAttributes, ...> and even if I did add it. What exactly should I add? UserAttributes[] or UserCreationAttributes[]? I hope we can fix this problem, I love this new TS approach.

hsource · 2020-08-02T06:07:44Z

I can't find a way to make this work when doing a create with include. e.g.:
class User extends Model<UserAttributes, UserCreationAttributes> {...}
class Project extends Model<ProjectAttributes, ProjectCreationAttributes> {...}
Project.hasMany(User);
Project.create({
  users: [{ name: 'project owner' }],
}, { include: [User] });
I get the error: "Object literal may only specify known properties, and 'users' does not exist in type ProjectCreationAttributes".

I'd add users into ProjectCreationAttributes but that's just a Optional<ProjectAttributes, ...> and even if I did add it. What exactly should I add? UserAttributes[] or UserCreationAttributes[]? I hope we can fix this problem, I love this new TS approach.

For includes, you should just manually add it via a cast. The old typings didn't really handle it either, and it'd be complicated to near-impossible to make this work automatically.

Here's what we do in our code

interface ProjectAttributesWithUser extends ProjectAttributes {
  User: UserAttributes;
}

const project = await Project.findAll({ ...someOptions, include: [User] }) as ProjectAttributesWithUser[];

hsource force-pushed the harry-strict-types branch from a39d6f5 to 007e975 Compare June 22, 2020 10:25

hsource force-pushed the harry-strict-types branch from 007e975 to 3e51c6b Compare June 22, 2020 10:44

hsource commented Jun 22, 2020

View reviewed changes

hsource force-pushed the harry-strict-types branch 2 times, most recently from 7821152 to ad54f72 Compare June 22, 2020 21:47

hsource marked this pull request as draft June 22, 2020 22:20

hsource force-pushed the harry-strict-types branch from ad54f72 to b3aaa8d Compare June 22, 2020 23:00

feat(types): added optional stricter typing for Model attributes

e4baea1

hsource force-pushed the harry-strict-types branch from b3aaa8d to e4baea1 Compare June 22, 2020 23:03

hsource marked this pull request as ready for review June 22, 2020 23:06

hsource marked this pull request as draft June 22, 2020 23:26

hsource added 2 commits June 22, 2020 17:00

fix(types): fixed ModelCtor type for models built with sequelize.define

4779c2d

fix: added types for increment/decrement, fixed upsert

b68e321

hsource marked this pull request as ready for review June 23, 2020 00:19

sushantdhiman approved these changes Jun 23, 2020

View reviewed changes

sushantdhiman requested review from SimonSchick, papb and eseliger June 23, 2020 04:42

sushantdhiman reviewed Jun 23, 2020

View reviewed changes

fix: made all Attribute generics optional, improved hooks typing

de1a7af

fix: reverted change to or object in where.ts

755836f

sushantdhiman merged commit 871157b into sequelize:master Jun 26, 2020

sushantdhiman added the released label Jun 26, 2020

intellix mentioned this pull request Jun 26, 2020

Type safe creation of models #11820

Closed

6 tasks

zaggino mentioned this pull request Jun 26, 2020

fix(types): init method should use all attributes #12431

Closed

This was referenced Jan 17, 2021

Sequelize v6 compatibility? sequelize/sequelize-typescript#844

Closed

v2.0.0: compiler error when using stricter typing Model attributes signature sequelize/sequelize-typescript#882

Closed

KapitanOczywisty mentioned this pull request Feb 6, 2021

fix(types): models with attributes cannot be used in some functions #13010

Merged

6 tasks

aryem mentioned this pull request Aug 28, 2021

Typed create #11001

Closed

hsource mentioned this pull request Dec 27, 2021

fix(types): allow use of sequelize.define models in types #13834

Closed

6 tasks

ephys mentioned this pull request Jan 15, 2022

refactor: convert utils to typescript #13782

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(types): added optional stricter typing for Model attributes #12405

feat(types): added optional stricter typing for Model attributes #12405

hsource commented Jun 22, 2020 •

edited

codecov bot commented Jun 22, 2020 •

edited

hsource left a comment

hsource Jun 22, 2020

hsource Jun 22, 2020

hsource commented Jun 22, 2020 •

edited

sushantdhiman commented Jun 23, 2020

sushantdhiman left a comment

sushantdhiman Jun 23, 2020

hsource Jun 23, 2020

sushantdhiman Jun 23, 2020

hsource Jun 25, 2020

hsource commented Jun 23, 2020 •

edited

sushantdhiman commented Jun 26, 2020

zaggino commented Jun 26, 2020

hsource commented Jun 27, 2020 •

edited

knoid commented Aug 2, 2020 •

edited

hsource commented Aug 2, 2020

feat(types): added optional stricter typing for Model attributes #12405

feat(types): added optional stricter typing for Model attributes #12405

Conversation

hsource commented Jun 22, 2020 • edited

Pull Request check-list

Description of change

Testing

Questions for reviewers

codecov bot commented Jun 22, 2020 • edited

Codecov Report

hsource left a comment

Choose a reason for hiding this comment

hsource Jun 22, 2020

Choose a reason for hiding this comment

hsource Jun 22, 2020

Choose a reason for hiding this comment

hsource commented Jun 22, 2020 • edited

sushantdhiman commented Jun 23, 2020

sushantdhiman left a comment

Choose a reason for hiding this comment

sushantdhiman Jun 23, 2020

Choose a reason for hiding this comment

hsource Jun 23, 2020

Choose a reason for hiding this comment

sushantdhiman Jun 23, 2020

Choose a reason for hiding this comment

hsource Jun 25, 2020

Choose a reason for hiding this comment

hsource commented Jun 23, 2020 • edited

sushantdhiman commented Jun 26, 2020

zaggino commented Jun 26, 2020

hsource commented Jun 27, 2020 • edited

knoid commented Aug 2, 2020 • edited

hsource commented Aug 2, 2020

hsource commented Jun 22, 2020 •

edited

codecov bot commented Jun 22, 2020 •

edited

hsource commented Jun 22, 2020 •

edited

hsource commented Jun 23, 2020 •

edited

hsource commented Jun 27, 2020 •

edited

knoid commented Aug 2, 2020 •

edited