Skip to content

Commit

Permalink
docs: add caveat regarding public class fields (#13877)
Browse files Browse the repository at this point in the history 
* docs: add caveat regarding public class fields

* docs: update link to class field caveat

* feat: improve public class field shadowing detection

* refactor: fix lint issue

* docs: fix code being incorrectly tagged as invalid

Co-authored-by: Guylian Cox <hello@guylian.me>
Co-authored-by: Zoé <zoe@ephys.dev>
  • Loading branch information
@sdepold @ephys
3 people committed Jan 1, 2022
1 parent 2e22f1b commit eceeabc
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 59 deletions.
63 changes: 63 additions & 0 deletions docs/manual/core-concepts/model-basics.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,69 @@ console.log(User === sequelize.models.User); // true

Internally, `sequelize.define` calls `Model.init`, so both approaches are essentially equivalent.

#### Caveat with Public Class Fields

Adding a [Public Class Field](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields) with the same name as one of the model's attribute is going to cause issues.
Sequelize adds a getter & a setter for each attribute defined through `Model.init`.
Adding a Public Class Field will shadow those getter and setters, blocking access to the model's actual data.

```typescript
// Invalid
class User extends Model {
id; // this field will shadow sequelize's getter & setter. It should be removed.
otherPublicField; // this field does not shadow anything. It is fine.
}

User.init({
id: {
type: DataTypes.INTEGER,
autoIncrement: true,
primaryKey: true
}
}, { sequelize });

const user = new User({ id: 1 });
user.id; // undefined
```

```typescript
// Valid
class User extends Model {
otherPublicField;
}

User.init({
id: {
type: DataTypes.INTEGER,
autoIncrement: true,
primaryKey: true
}
}, { sequelize });

const user = new User({ id: 1 });
user.id; // 1
```

In TypeScript, you can add typing information without adding an actual public class field by using the `declare` keyword:

```typescript
// Valid
class User extends Model {
declare id: number; // this is ok! The 'declare' keyword ensures this field will not be emitted by TypeScript.
}

User.init({
id: {
type: DataTypes.INTEGER,
autoIncrement: true,
primaryKey: true
}
}, { sequelize });

const user = new User({ id: 1 });
user.id; // 1
```

## Table name inference

Observe that, in both methods above, the table name (`Users`) was never explicitly defined. However, the model name was given (`User`).
Expand Down
84 changes: 47 additions & 37 deletions docs/manual/other-topics/typescript.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,19 @@ In order to avoid installation bloat for non TS users, you must install the foll

Example of a minimal TypeScript project with strict type-checking for attributes.

**Important**: You must use `declare` on your class properties typings to ensure TypeScript does not emit those class properties.
See [Caveat with Public Class Fields](./model-basics.html#caveat-with-public-class-fields)

**NOTE:** Keep the following code in sync with `/types/test/typescriptDocs/ModelInit.ts` to ensure it typechecks correctly.

```ts
```typescript
/**
* Keep this file in sync with the code in the "Usage" section in typescript.md
*/
import {
Sequelize,
Model,
ModelDefined,
DataTypes,
HasManyGetAssociationsMixin,
HasManyAddAssociationMixin,
HasManyHasAssociationMixin,
Association,
HasManyCountAssociationsMixin,
HasManyCreateAssociationMixin,
Optional,
Association, DataTypes, HasManyAddAssociationMixin, HasManyCountAssociationsMixin,
HasManyCreateAssociationMixin, HasManyGetAssociationsMixin, HasManyHasAssociationMixin, Model,
ModelDefined, Optional, Sequelize
} from "sequelize";

const sequelize = new Sequelize("mysql://root:asd123@localhost:3306/mydb");
Expand All @@ -46,28 +44,28 @@ interface UserCreationAttributes extends Optional<UserAttributes, "id"> {}

class User extends Model<UserAttributes, UserCreationAttributes>
implements UserAttributes {
public id!: number; // Note that the `null assertion` `!` is required in strict mode.
public name!: string;
public preferredName!: string | null; // for nullable fields
declare id: number; // Note that the `null assertion` `!` is required in strict mode.
declare name: string;
declare preferredName: string | null; // for nullable fields

// timestamps!
public readonly createdAt!: Date;
public readonly updatedAt!: Date;
declare readonly createdAt: Date;
declare readonly updatedAt: Date;

// Since TS cannot determine model association at compile time
// we have to declare them here purely virtually
// these will not exist until `Model.init` was called.
public getProjects!: HasManyGetAssociationsMixin<Project>; // Note the null assertions!
public addProject!: HasManyAddAssociationMixin<Project, number>;
public hasProject!: HasManyHasAssociationMixin<Project, number>;
public countProjects!: HasManyCountAssociationsMixin;
public createProject!: HasManyCreateAssociationMixin<Project>;
declare getProjects: HasManyGetAssociationsMixin<Project>; // Note the null assertions!
declare addProject: HasManyAddAssociationMixin<Project, number>;
declare hasProject: HasManyHasAssociationMixin<Project, number>;
declare countProjects: HasManyCountAssociationsMixin;
declare createProject: HasManyCreateAssociationMixin<Project>;

// You can also pre-declare possible inclusions, these will only be populated if you
// actively include a relation.
public readonly projects?: Project[]; // Note this is optional since it's only populated when explicitly requested in code
declare readonly projects?: Project[]; // Note this is optional since it's only populated when explicitly requested in code

public static associations: {
declare static associations: {
projects: Association<User, Project>;
};
}
Expand All @@ -76,18 +74,19 @@ interface ProjectAttributes {
id: number;
ownerId: number;
name: string;
description?: string;
}

interface ProjectCreationAttributes extends Optional<ProjectAttributes, "id"> {}

class Project extends Model<ProjectAttributes, ProjectCreationAttributes>
implements ProjectAttributes {
public id!: number;
public ownerId!: number;
public name!: string;
declare id: number;
declare ownerId: number;
declare name: string;

public readonly createdAt!: Date;
public readonly updatedAt!: Date;
declare readonly createdAt: Date;
declare readonly updatedAt: Date;
}

interface AddressAttributes {
Expand All @@ -98,11 +97,11 @@ interface AddressAttributes {
// You can write `extends Model<AddressAttributes, AddressAttributes>` instead,
// but that will do the exact same thing as below
class Address extends Model<AddressAttributes> implements AddressAttributes {
public userId!: number;
public address!: string;
declare userId: number;
declare address: string;

public readonly createdAt!: Date;
public readonly updatedAt!: Date;
declare readonly createdAt: Date;
declare readonly updatedAt: Date;
}

// You can also define modules in a functional way
Expand All @@ -113,7 +112,8 @@ interface NoteAttributes {
}

// You can also set multiple attributes optional at once
interface NoteCreationAttributes extends Optional<NoteAttributes, 'id' | 'title'> {};
interface NoteCreationAttributes
extends Optional<NoteAttributes, "id" | "title"> {}

Project.init(
{
Expand All @@ -130,6 +130,10 @@ Project.init(
type: new DataTypes.STRING(128),
allowNull: false,
},
description: {
type: new DataTypes.STRING(128),
allowNull: true,
},
},
{
sequelize,
Expand Down Expand Up @@ -180,7 +184,7 @@ const Note: ModelDefined<
NoteAttributes,
NoteCreationAttributes
> = sequelize.define(
'Note',
"Note",
{
id: {
type: DataTypes.INTEGER.UNSIGNED,
Expand All @@ -189,15 +193,15 @@ const Note: ModelDefined<
},
title: {
type: new DataTypes.STRING(64),
defaultValue: 'Unnamed Note',
defaultValue: "Unnamed Note",
},
content: {
type: new DataTypes.STRING(4096),
allowNull: false,
},
},
{
tableName: 'notes',
tableName: "notes",
}
);

Expand All @@ -220,6 +224,7 @@ async function doStuffWithUser() {

const project = await newUser.createProject({
name: "first!",
ownerId: 123,
});

const ourUser = await User.findByPk(1, {
Expand All @@ -231,6 +236,11 @@ async function doStuffWithUser() {
// the model or not
console.log(ourUser.projects![0].name);
}

(async () => {
await sequelize.sync();
await doStuffWithUser();
})();
```

### Usage without strict types for attributes
Expand Down
25 changes: 25 additions & 0 deletions lib/model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,29 @@ class Model {
* @param {Array} [options.include] an array of include options - Used to build prefetched/included model instances. See `set`
*/
constructor(values = {}, options = {}) {
if (!this.constructor._overwrittenAttributesChecked) {
this.constructor._overwrittenAttributesChecked = true;

// setTimeout is hacky but necessary.
// Public Class Fields declared by descendants of this class
// will not be available until after their call to super, so after
// this constructor is done running.
setTimeout(() => {
const overwrittenAttributes = [];
for (const key of Object.keys(this.constructor._attributeManipulation)) {
if (Object.prototype.hasOwnProperty.call(this, key)) {
overwrittenAttributes.push(key);
}
}

if (overwrittenAttributes.length > 0) {
logger.warn(`Model ${JSON.stringify(this.constructor.name)} is declaring public class fields for attribute(s): ${overwrittenAttributes.map(attr => JSON.stringify(attr)).join(', ')}.` +
'\nThese class fields are shadowing Sequelize\'s attribute getters & setters.' +
'\nSee https://sequelize.org/main/manual/model-basics.html#caveat-with-public-class-fields');
}
}, 0);
}

options = {
isNewRecord: true,
_schema: this.constructor._schema,
Expand Down Expand Up @@ -1268,6 +1291,8 @@ class Model {

this._hasPrimaryKeys = this.primaryKeyAttributes.length > 0;
this._isPrimaryKey = key => this.primaryKeyAttributes.includes(key);

this._attributeManipulation = attributeManipulation;
}

/**
Expand Down
49 changes: 27 additions & 22 deletions types/test/typescriptDocs/ModelInit.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,28 +21,28 @@ interface UserCreationAttributes extends Optional<UserAttributes, "id"> {}

class User extends Model<UserAttributes, UserCreationAttributes>
implements UserAttributes {
public id!: number; // Note that the `null assertion` `!` is required in strict mode.
public name!: string;
public preferredName!: string | null; // for nullable fields
declare id: number; // Note that the `null assertion` `!` is required in strict mode.
declare name: string;
declare preferredName: string | null; // for nullable fields

// timestamps!
public readonly createdAt!: Date;
public readonly updatedAt!: Date;
declare readonly createdAt: Date;
declare readonly updatedAt: Date;

// Since TS cannot determine model association at compile time
// we have to declare them here purely virtually
// these will not exist until `Model.init` was called.
public getProjects!: HasManyGetAssociationsMixin<Project>; // Note the null assertions!
public addProject!: HasManyAddAssociationMixin<Project, number>;
public hasProject!: HasManyHasAssociationMixin<Project, number>;
public countProjects!: HasManyCountAssociationsMixin;
public createProject!: HasManyCreateAssociationMixin<Project>;
declare getProjects: HasManyGetAssociationsMixin<Project>; // Note the null assertions!
declare addProject: HasManyAddAssociationMixin<Project, number>;
declare hasProject: HasManyHasAssociationMixin<Project, number>;
declare countProjects: HasManyCountAssociationsMixin;
declare createProject: HasManyCreateAssociationMixin<Project>;

// You can also pre-declare possible inclusions, these will only be populated if you
// actively include a relation.
public readonly projects?: Project[]; // Note this is optional since it's only populated when explicitly requested in code
declare readonly projects?: Project[]; // Note this is optional since it's only populated when explicitly requested in code

public static associations: {
declare static associations: {
projects: Association<User, Project>;
};
}
Expand All @@ -58,12 +58,12 @@ interface ProjectCreationAttributes extends Optional<ProjectAttributes, "id"> {}

class Project extends Model<ProjectAttributes, ProjectCreationAttributes>
implements ProjectAttributes {
public id!: number;
public ownerId!: number;
public name!: string;
declare id: number;
declare ownerId: number;
declare name: string;

public readonly createdAt!: Date;
public readonly updatedAt!: Date;
declare readonly createdAt: Date;
declare readonly updatedAt: Date;
}

interface AddressAttributes {
Expand All @@ -74,11 +74,11 @@ interface AddressAttributes {
// You can write `extends Model<AddressAttributes, AddressAttributes>` instead,
// but that will do the exact same thing as below
class Address extends Model<AddressAttributes> implements AddressAttributes {
public userId!: number;
public address!: string;
declare userId: number;
declare address: string;

public readonly createdAt!: Date;
public readonly updatedAt!: Date;
declare readonly createdAt: Date;
declare readonly updatedAt: Date;
}

// You can also define modules in a functional way
Expand Down Expand Up @@ -212,4 +212,9 @@ async function doStuffWithUser() {
// Note the `!` null assertion since TS can't know if we included
// the model or not
console.log(ourUser.projects![0].name);
}
}

(async () => {
await sequelize.sync();
await doStuffWithUser();
})();

0 comments on commit eceeabc

Please sign in to comment.