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

feat(types): simplify Model definitions #14091

Closed
wants to merge 6 commits into from

Conversation

ephys
Copy link
Member

@ephys ephys commented Feb 11, 2022

Pull Request Checklist

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

Description Of Change

Pretty large PR, it's a typing breaking change that makes InferAttributes the default way of handling attribute typing in v7.

So instead of writing:

class User extends Model<InferAttributes<User>, InferCreationAttributes<User>> {}

You now write:

class User extends Model<User> {}

Instead of writing:

interface UserAttributes {
  id: number;
  name: string;
}

interface UserCreationAttributes extends Optional<UserAttributes, 'id'> {}

interface UserInstance
  extends Model<UserAttributes, UserCreationAttributes>,
    UserAttributes {}

const UserModel = sequelize.define<UserInstance>('User', {
  id: {
    primaryKey: true,
    type: DataTypes.INTEGER.UNSIGNED,
  },
  name: {
    type: DataTypes.STRING,
  }
});

You now write:

interface IUserModel extends Model<IUserModel> {
  id: CreationOptional<number>;
  name: string;
}

const UserModel = sequelize.define<IUserModel>('User', {
  id: {
    primaryKey: true,
    type: DataTypes.INTEGER.UNSIGNED,
  },
  name: {
    type: DataTypes.STRING,
  },
});

Extra changes:

  • Sequelize.define now uses InferAttributes too.
  • InferAttributes & co are purely internal in v7, they are not exported anymore.
  • Model['_attributes'] has been replaced with Model[AttributeSymbol] to prevent key collision in child models.
  • Attributes<Model> now supports ModelStatic too! Usefull when used with Sequelize.define

The documentation has been updated to reflect this (as a result it's also much, much shorter)
The breaking change has been documented

@ephys ephys added breaking change For issues and PRs. Changes that break compatibility and require a major version increment. type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. labels Feb 11, 2022
@ephys ephys self-assigned this Feb 11, 2022
@ephys
Copy link
Member Author

ephys commented Feb 11, 2022

So this is interesting! The same code that was breaking in #14032 is now breaking here. I'll continue on that other PR to find a solution

@WikiRik
Copy link
Member

WikiRik commented Feb 11, 2022

Is this part of one of the RFCs you proposed?

@ephys
Copy link
Member Author

ephys commented Feb 11, 2022

It was in my personal wishlist but I never opened an actual RFC for this breaking change
If there is any opposition or concerns, don't hesitate to talk about it in this PR :)

@WikiRik
Copy link
Member

WikiRik commented Feb 11, 2022

Ah, I read something about InferAttributes somewhere and was just curious. Will read into this more later but shorter in most cases is better so I'm happy about this

@ephys
Copy link
Member Author

ephys commented Feb 11, 2022

InferAttributes is the type I introduced in v6 a few weeks ago to reduce model typing boilerplate without causing a breaking change (#13909)

Since it looks like it's stable (I can't say how many people have used it yet, but it worked in my different projects), I'm going one step further and implementing that missing breaking change :)

@sdepold
Copy link
Member

sdepold commented Feb 11, 2022

Simpler is always better, hm? :)

class User extends Model<User> {}

What is this User referring to? Another type? Or is the User class?

```typescript
import { Model, CreationOptional } from 'sequelize';

class User extends Model<User> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I have never used this syntax before, I have no clue but I think one would have to call the Model's init function afterwards somehow?

Can you add that to the example?

@ephys
Copy link
Member Author

ephys commented Feb 11, 2022

What is this User referring to? Another type? Or is the User class?

Itself! It generates its attribute typings from its own class

@sdepold
Copy link
Member

sdepold commented Feb 11, 2022

What is this User referring to? Another type? Or is the User class?

Itself! It generates its attribute typings from its own class

Pure magic! I love it!

@ephys
Copy link
Member Author

ephys commented Feb 20, 2022

I will draft this until #14125 has been resolved


We also need a way to define support for includes in Model.create, e.g. the following should not be an error:

class User extends Model<User> {
  declare groupId: number;
  declare group?: UserGroup;
}

await User2.create({
  id: 2,
  group: { id: 1 },
}, {
  include: [User.associations.group],
});

I think #14302 can help, and this is related to #14129

@ephys
Copy link
Member Author

ephys commented Apr 13, 2022

This PR has been in limbo for 2 months but it's blocked by #14302, which itself is blocked by #14280 (which in turn is blocked by #14333)

In the meantime I've replicated its non-blocked changes in #14333, to reduce the scope of this PR

@WikiRik WikiRik added the blocked label Nov 8, 2022
@ephys
Copy link
Member Author

ephys commented Feb 11, 2024

I'll revisit this change when the rest of the project is ready for it

@ephys ephys closed this Feb 11, 2024
@ephys ephys deleted the ephys/remove-deprecated-model-types branch February 11, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked breaking change For issues and PRs. Changes that break compatibility and require a major version increment. type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants