How to fix the incompatibility with Public Class Fields

[ephys]

Yet another RFC because this would be a large change. Promise at some point I'll start writing actual code instead of just describing how it could work

The Problem

There is an incompatibility in Sequelize between Public Class Fields & Sequelize attributes.

I've documented it in greater details here: #13861

Here are also a few issues I found that are caused by this:

• Intance model properties are undefined sequelize-typescript#778
• undefined attributes following typescript example #11675
• Typescript unusable & Typescript tutorial completely broken "sequelize": "^6.3.4", #12603
• Can't insert records when using Typescript. #11032
• Mention known issues involving Babel + Sequelize in docs #11494
• Sequelize with TypeScript - class fields undefined #10579
• Model values are undefined while data values are #13527

To sum up the problem, if I do the following:

class User extends Model {
  name;
}

User.init({
  name: {
    type: DataTypes.STRING,
    allowNull: false
  }
}, { sequelize });

const user = User.build({ name: 'zoé' });

user.name won't be equal to 'zoé', it will be undefined.

This is because User.init adds get name() & set name() to User.prototype, which the public class field then shadows.

The current solution

The only way, currently, to avoid the problem is to not add a public class field that share the same name as one of the model's attribute. In TypeScript this is done using declare on a public class field:

class User extends Model
{
  declare public id: number;
}

transpiles to:

class User extends Model
{
}

And, thankfully, declare is also compatible with decorators:

class User extends Model
{
  @Column
  declare public id: number;
}

transpiles to:

class User extends Model {
  // no `id` public class field has been added even though it's being decorated (good for us)
}
__decorate([
    Column
], User.prototype, "id", void 0);

The unresolved Problem

Babel emits public class fields when they're decorated, even if they're tagged with declare. Meaning projects like sequelize-typescript break when using babel instead of tsc.

As public class fields become more mainstream, and decorators move slowly towards standardization, it's very likely that transpilers will start outputting public class fields if they're decorated. At some point, decorators won't be transpiled at all.

At that point, sequelize will break down.

To be future-proof, We need to become compatible with public class fields.

But how?

I've been searching for a solution, and so far the only I've found is: drop Model#dataValues, store data directly in the model.

Instead of

User {
  dataValues: {
    id: 6,
    name: 'Johnny',
    preferredName: 'John',
    updatedAt: 2021-12-30T10:52:05.115Z,
    createdAt: 2021-12-30T10:52:05.115Z
  },
  _previousDataValues: {
    name: 'Johnny',
    preferredName: 'John',
    id: 6,
    createdAt: 2021-12-30T10:52:05.115Z,
    updatedAt: 2021-12-30T10:52:05.115Z
  },
  uniqno: 1,
  _changed: Set(0) {},
  _options: {
    isNewRecord: true,
    _schema: null,
    _schemaDelimiter: '',
    attributes: undefined,
    include: undefined,
    raw: undefined,
    silent: undefined
  },
  isNewRecord: false
}

We'd have

User {
  id: 6,
  name: 'Johnny',
  preferredName: 'John',
  updatedAt: 2021-12-30T10:52:05.115Z,
  createdAt: 2021-12-30T10:52:05.115Z,
  #previousDataValues: {
    name: 'Johnny',
    preferredName: 'John',
    id: 6,
    createdAt: 2021-12-30T10:52:05.115Z,
    updatedAt: 2021-12-30T10:52:05.115Z
  },
  [Symbol('uniqno')]: 1,
  #options: {
    isNewRecord: true,
    _schema: null,
    _schemaDelimiter: '',
    attributes: undefined,
    include: undefined,
    raw: undefined,
    silent: undefined
  },
  #changed: Set(0) {},
  get isNewRecord(): true,
}

• dataValues is removed, all properties are at the top level.
• Because the setters have been removed, we can't detect dirtyness based on calling the setter. It would be replaced by doing a diff between previousDataValues & User instead.
• To reduce key conflict between sequelize's properties, and user-defined ones, all properties added by Sequelize are added either as private fields (#previousDataValues) or using symbols (Symbol('uniqno')).

Detecting changes (Model#save())

As specified above, with the new system, model dirty-ness would be detected by comparing previous and current values, instead of detected by calling a setter.

For that end, we could either:

• Do a shallow equal of previous and current values (faster).
• Do a deep equal of previous and current values (slower, but would detect mutations. Also means previousDataValues would need to be a deep copy of current data values to avoid accidental mutations).

A compare algorithm needs to support (will update as I discover them):

• Date
• Buffer
• Arrays
• objects

A deep diff would resolves issues like:

• Updating instance of a buffer does not persist Blob #13801
• postgres .save() not able to detect change in Array fields #13833
• Cannot update value inside JSON #12960
• JSON property dirty checking broken in v6 #13006
• updating an array with unshift does not work #11247

What about Model#changed

It would still be there, as a mechanism to bypass the diff (force marking something as dirty when our deep equal implementation fails to detect it), but it would only be able to set it to true. As such I propose a method rename:

• setChanged(key): this: forces the model to consider an attribute as changed
• hasChanged(key): boolean: returns whether the model considers the attribute changed (first by checking if it's force marked as dirty, then by doing a deep equal).

Things to sort first

We're blocked by the TypeScript Migration. We should migrate the Model class before working on this.

Attributes can accept multiple different types. Making a deep equal test difficult. DATE can sometimes be Date, sometimes be string. BIGINT can sometimes be number, sometimes bigint, sometimes string.

We'll need to provide a mechanism to ensure the type is always the same, and the one the user expects. I've started thoughts on this here https://gist.github.com/ephys/50a6a05be7322c64645be716fea6186a#support-for-alternative-js-types

[ephys]

We can resolve the issue by adding this piece of code at the end of Model.build:

// If the user declared class properties with the same name as one of the model's attributes (which happens due to typing and decorators),
// that class property will shadow the attribute's getter/setter (which lives on the prototype).
// This deletes that class property.
for (const attributeName of this.modelDefinition.attributes.keys()) {
  delete instance[attributeName];
}

This means Model.build() is safer to use than new Model(). If we implement this I propose to deprecate new Model() and force users to always use Model.build().

I'll open a PR once #15431 has been merged

About the deprecation notice

ℹ️ The deprecation notice added in Sequelize 7 points to this comment. If you don't understand why you're getting the notice, you're likely in one of the following two situations:

Option 1: You're instantiating your model directly

Say you have a model called User, you should not be creating new instances of it using new User(). Instead, prefer using the "build" static method that is available on all models. Write User.build().

Option 2: You have a custom constructor on your model

Sequelize checks that you're not using instantiating your model directly by passing a secret to the constructor parameter. If you want to write a custom constructor in your model, make sure to pass the third parameter to the parent constructor, like this:

class User extends Model {
  constructor(values, options, secret) {
    super(values, options, secret);

    // your custom constructor logic
  }
}