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

[RFC] Rework association methods for better TS support #14302

Open
2 tasks done
ephys opened this issue Dec 7, 2021 · 6 comments
Open
2 tasks done

[RFC] Rework association methods for better TS support #14302

ephys opened this issue Dec 7, 2021 · 6 comments
Assignees
Labels
RFC Request for comments regarding breaking or large changes topic: associations Features & issues related to association querying type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.

Comments

@ephys
Copy link
Member

ephys commented Dec 7, 2021

Is your feature request related to a problem? Please describe.

The way association methods are generated today is cumbersome when using TypeScript.

sequelize-typescript solves this by adding $get, $count, etc methods.

Describe the solution you'd like

I think we could improve developer ergonomics by grouping generated methods in one object.

So instead of looking like this

import {
  Model,
  HasManyGetAssociationsMixin,
  HasManyAddAssociationMixin,
  HasManyHasAssociationMixin,
  HasManyCountAssociationsMixin,
  HasManyCreateAssociationMixin,
  Association,
} from "sequelize";

class User extends Model {
  declare getProjects: HasManyGetAssociationsMixin<Project>; // Note the null assertions!
  declare addProject: HasManyAddAssociationMixin<Project, number>;
  declare addProjects: HasManyAddAssociationsMixin<Project, number>;
  declare setProjects: HasManySetAssociationsMixin<Project, number>;
  declare removeProject: HasManyRemoveAssociationMixin<Project, number>;
  declare removeProjects: HasManyRemoveAssociationsMixin<Project, number>;
  declare hasProject: HasManyHasAssociationMixin<Project, number>;
  declare hasProjects: HasManyHasAssociationsMixin<Project, number>;
  declare countProjects: HasManyCountAssociationsMixin;
  declare createProject: HasManyCreateAssociationMixin<Project, 'ownerId'>;
  declare readonly projects?: Project[];

  declare static associations: {
    projects: Association<User, Project>;
  };
}

The association would look like this:

import { Model, HasManyAttribute } from "sequelize";

class User extends Model {
  declare readonly projects: HasManyAttribute<Project, number>;

  // static associations typings now smart enough, no need to set them :)
}

with HasManyAttribute looking like this

// HasManyAttribute definition (provided by sequelize)
class HasManyAttribute<Model, PrimaryKey> {
  value: Project[] | undefined; // when eager loaded through association. `undefined` when not loaded.
  findOne: HasManyGetAssociationMixin<Model>;
  findAll: HasManyGetAssociationsMixin<Model>;
  add: HasManyAddAssociationMixin<Model, PrimaryKey>;
  bulkAdd: HasManyAddAssociationsMixin<Model, PrimaryKey>;
  setAll: HasManySetAssociationsMixin<Model, PrimaryKey>;
  remove: HasManyRemoveAssociationMixin<Model, PrimaryKey>;
  bulkRemove: HasManyRemoveAssociationsMixin<Model, PrimaryKey>;
  has: HasManyHasAssociationMixin<Model, PrimaryKey>;
  hasMany: HasManyHasAssociationsMixin<Model, PrimaryKey>;
  count: HasManyCountAssociationsMixin;
  create: HasManyCreateAssociationMixin;
}

This would also open the door for a clean decorator support

import { Model, HasManyAttribute, HasMany } from "sequelize";

class User extends Model {
  @HasMany(Project)
  public readonly projects!: HasManyAttribute<Project, number>;
}

Why should this be in Sequelize

I think this way is less verbode, less error prone, and improves typescript users ergonomics.

This solution also enables us to add smart typings for the associations static property. Demo here

Note that this would be a pretty big breaking change.

Describe alternatives/workarounds you've considered

The way sequelize-typescript does it. But this solution lets us use different typings for each association type.

Feature Request Checklist

Is this feature dialect-specific?

  • No. This feature is relevant to Sequelize as a whole.

Would you be willing to implement this feature by submitting a Pull Request?

  • Yes, I have the time and I know how to start.
@ephys ephys changed the title RFC: association methods in v7 [RFC] Rework association methods for better TS support Dec 23, 2021
@ephys ephys transferred this issue from sequelize/sequelize Dec 23, 2021
@ephys ephys added the RFC Request for comments regarding breaking or large changes label Dec 23, 2021
@ephys ephys self-assigned this Dec 23, 2021
@ephys
Copy link
Member Author

ephys commented Jan 9, 2022

This RFC would also allow us to resolve #7621, as pluralization is used in associations method names

@ephys
Copy link
Member Author

ephys commented Jan 28, 2022

Note from https://gist.github.com/ephys/078ee6a2eb31dc209f0de6cea95316e7:

This would also simplify adding QueryBuilder support:

// non qb version
user.projects.findAll()

// qb version
user.projects.select()
  .orderBy('id', 'ASC')
  .findAll()

@mmahalwy
Copy link
Contributor

mmahalwy commented Mar 7, 2022

This would be amazing! Would be a game-changer to reducing developer mistakes.

@ephys
Copy link
Member Author

ephys commented Mar 17, 2022

Caveat: It's not possible to infer the types of static class fields based on generics.

Model.rawAttributes has been replaced with Model.getAttributes() for this reason.

I think we could deprecate and replace Model.associations with Model.getAssociations(). And also add Model.getAssociation(associationName): Association - which would throw if the association doesn't exist.

@jthoward64
Copy link

Is this planned to be held off until v8/beyond or would work on this be accepted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comments regarding breaking or large changes topic: associations Features & issues related to association querying type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
Status: Blocked (any)
Development

No branches or pull requests

3 participants