Make instances instanceof Model, with ES6 classes

[felixfbecker]

This PR is a reimplementation of #5877 with ES6 classes. Reasoning is performance and that 4.0 will probably require Node >=4 or even 6.

[felixfbecker]

Apparently native modules do not compile, any help?

[sushantdhiman]

Try adding the addons for g++ and c++, something like

addons:
    apt:
     sources:
       - llvm-toolchain-precise-3.6
       - ubuntu-toolchain-r-test
     packages:
       - clang-3.6
       - g++-4.8

[felixfbecker]

@sushantdhiman didnt work :/

[mickhansen]

diff too large :(

[mickhansen]

Can you rebase this? Might help with the tests

[janmeier]

Then we promise to review it quickly! :)

[felixfbecker]

I will try, hopefully there were no changes in model.js or instance.js... It's a pain to rebase this because for git it's just one huge delete and one huge insert.

[mickhansen]

@felixfbecker is there a document describing your design and thoughts behind this anywhere? Syntax examples etc.

[felixfbecker]

@mickhansen I outlined my thoughts in the initial PR #5877, there are also some comments linked

[mickhansen]

@felixfbecker Thanks i've looked through a bit of it.
Attributes, name, etc would have to be statics, so with es7 static and with es6 simply add it as a property after the fact (which could be done by define()).

[felixfbecker]

I think I cought everything. Had to essentially reimplement every single commit on master since my PR.

@mickhansen everything that was on the prototype of Model is now a static. The name is set by sequelize.define(), and if you subclass Model yourself, the name will be the name you give your class.

[mickhansen]

@felixfbecker We'd need a way to overwrite name, cause i use lowercase model names but i'd like pascal case class names.

We don't need to main .Instance for BC.

[mickhansen]

This can be removed completely.

[mickhansen]

I assume you made no changes when merging Instance/Model other than doing a single file and doing all Model methods as static, correct?

What is the syntax for defining attributes, model name, table name, underscored etc, with extends Sequelize.Model? (assuming es6 so no decorators or static props)

[mickhansen]

Ideally i would love to work on a doc describing how to upgrade from ES5 model to both ES6 and ES7 models (defining needed properties), this doc could be exploratory and we could work from that.

[mickhansen]

But we could likely merge this for the sake of no-rebases and then move forward with the design after, as long as we arent cutting releases theres room to maneuver.

[felixfbecker]

@mickhansen

We'd need a way to overwrite name, cause i use lowercase model names but i'd like pascal case class names.

We could pass the model name to Model.init(). But this of course will override the native name property on classes, so YourClass.name === 'yourClass'. I find it a bit redundant to have to pass the name everytime, like:

class User extends Sequelize.Model {}
Model.init(User.name, {}, {}, sequelize.modelManager)

Then again, Model.init() would be more low-level API for decorator libraries and I would expect ES5/6 users to continue to use sequelize.define(). For define() you can of course always do const User = sequelize.define('user', {...})

I assume you made no changes when merging Instance/Model other than doing a single file and doing all Model methods as static, correct?

Basically yes. Plus I changed sequelize.define() and all instanceof Model checks I could find.

What is the syntax for defining attributes, model name, table name, underscored etc, with extends Sequelize.Model ?

Model.init() is the static constructor of the model classes. It takes the attributes, options and the modelManager. I'm a bit unhappy with the modelManager as the method does nothing with it as set this.modelManager = modelManager. Is this needed? I would prefer to make the Model not know about the modelManager and add the model with something like sequelize.addModel(Model).

Ideally i would love to work on a doc describing how to upgrade from ES5 model to both ES6 and ES7 models (defining needed properties), this doc could be exploratory and we could work from that.

So you would make Model.init() a high-level API? I would propose a section in the docs about how to use sequelize in different languages, a guide for the old style with define(), a guide for ES6 with Model.init(), and a guide for TypeScript users with decorators etc. I expect many users to stay on the define() style though because atm ES6 classes don't provide that much of a benefit. The potential really lays in decorators.

But we could likely merge this for the sake of no-rebases and then move forward with the design after, as long as we arent cutting releases theres room to maneuver.

That would be nice, so other PRs can target the class implementation. So it is decided that Node 4/6 is required (for classes)? Do you want me to squash or keep it in multiple commits?

I would then go ahead and refactor other parts of the library to classes and potentially arrow functions.
Also, I would take on #5919 and remove (old?) $ methods and aliases.

[mickhansen]

YourClass.name === 'yourClass'

Should be sufficient for now.

I would prefer to make the Model not know about the modelManager and add the model with something like sequelize.addModel(Model).

That would be the goal, addModel would then return a model aswell, since we need a version of the model that is aware of sequelize.

So you would make Model.init() a high-level API

No, model.init is a mapper for define (imo), i'd look to a better design for ES6 and a design for ES7 we could strive to in the future.

The design could be ES7 decorators that would work for ES6 aswell (although without the decorator syntax), or it could be Model.attributes = {}.

I agree that most of the power will come with static props and decorators, but i think it makes sense to have an intermediate design aswell (since static props and decorators need to map to that in any case).

We've pretty much decided on Node 4.0+ atleast, and i don't think we'll go for Node 6.0+ only.

I'll squash when i merge.

[felixfbecker]

@mickhansen

That would be the goal, addModel would then return a model aswell, since we need a version of the model that is aware of sequelize.

I don't know what you mean with addModel() needs to return a model, I thought of something like this:

class User extends Sequelize.Model {
  public username: string;
  myMethod() {
    return 'hello';
  }
}
User.init({username: Sequelize.STRING}, {tableName: 'users'}) // attach metadata, this can alternatively be done with decorators
sequelize.addModel(Model) // adds the model to the modelManager and sets User.modelManager

No, model.init is a mapper for define (imo), i'd look to a better design for ES6 and a design for ES7 we could strive to in the future.

The design could be ES7 decorators that would work for ES6 aswell (although without the decorator syntax), or it could be Model.attributes = {} .

The idea is that Model.init() is simply a static constructor and any decorator can invoke that to attach the metadata. A class decorator in ES7 simply takes the class and the attributes and options and calls init(). In TypeScript, property decorators can assign metadata on the class with symbols (and we can also get some type metadata automatically through this). The class decorator can then scan the class for these symbols and fill out the attributes object. So I would propose that Model.init() is that intermediate API and decorators can be implemented in third-party modules.

define() is a factory that creates a new subclass of Model, sets a name on it, calls Model.init() with the attributes and options and returns it. It is nice because you can define a Model in one function call, but the con is that you don't get any type hints from your editor for that class because it is dynamically created and not declared.

[felixfbecker]

How can the build fail because of style issues (mixed double/single quotes) if I only copy+pasted code?

[mickhansen]

I don't know what you mean with addModel() needs to return a model, I thought of something like this:

If we're going to be doing ES6 classes they should be decoupled.
Calling addModel should not affect the base instance since it should ideally be possible to use the same model for multiple connections.

A class decorator in ES7 simply takes the class and the attributes and options and calls init()

I thought all decorators were basically just functions?

[felixfbecker]

If we're going to be doing ES6 classes they should be decoupled.
Calling addModel should not affect the base instance since it should ideally be possible to use the same model for multiple connections.

If you want to support that scenario... Imo it should also be supported that I only define my class, set the connection/modelManager whatever that should be used on it and then use this class directly. I do not want to use the return value of some function as my model class... Eg it should be export class MyModel and not export const MyModel = sequelize.addModel(MyModelClass).

I think having one sequlize connection instance is the most common use case. Cloning the model class seems overkill to me to implement this feature and is tricky to do in JS (a class is just a function). I think if people want to use different connections with one model class, there should be an option to pass it to every function that needs a connection, eg find(), save(), ...
So you could either a) set one (default) sequelize connection on the model to be used or b) pass it as an option to the methods.

[janmeier]

If you want I think it would be fine to split this into smaller commits, but if not, squashing is fine by me as well

[janmeier]

I'm hoping to do a final review and merge over the weekend :)

[janmeier]

Minor thing, but couldn't this be instanceof?

[felixfbecker]

yes.

[janmeier]

@felixfbecker I've reviewed the changes to model.js locally, so I'm ready to merge - Do you want to do the squashing dance, or do you want me to mono-squash it?

[felixfbecker]

@janmeier I cannot think of a good way to squash this into more than one commit, so go ahead :)

[janmeier]

Thanks for the big effort @felixfbecker 🎉 🎉 🎉

[sushantdhiman]

@felixfbecker gz

[mickhansen]

Yay! Thanks a bunch @felixfbecker

[nervgh]

Is there docs, how to use extends Sequelize.Model syntax in the 4.0.0.-0 version?

I have this code:

// schema is sequelize instance

let Model = schema.define('news', {
  title: {
    type: Sequelize.STRING,
    defaultValue: null
  },
  content: {
    type: Sequelize.TEXT,
    allowNull: true
  }
});

How I could specify:

1. table' name
2. table' columns

class Model extends Sequelize.Model {
}

? What I need to do?

Thanks.

[felixfbecker]

@nervgh There is no documentation yet because the API is not finished and will probably change a lot. Currently you have to call

Model.init(attributes, options, sequelize.modelManager);
sequelize.modelManager.add(Model)

options also needs to include sequelize.

Please note though that sequelize.define() still works.

[nervgh]

Please note though that sequelize.define() still works.

Yes, I know, I do it. But I want use inheritance for overriding a couple of built-in methods:

class Model extends Sequelize.Model {
  save(options) {
    // my code here
    return super.save(options);
  }
}

@felixfbecker thank you ✌️ I will try your recommendations.

[felixfbecker]

@nervgh You can do that with define() too by assigning to the prototype. define() returns a subclass of Model.

[OLDIN]

@nervgh

// user.js 
export default class User {

  constructor(sequelize, types) {

    return sequelize.define("users", {
      steamid: types.STRING(20),
      name: types.STRING(30),
      avatar: types.STRING(30),
      coins: types.INTEGER,
      permissions: types.INTEGER(1),
      banned: types.BOOLEAN
    });

  }

}

// index.js
import userModel from './models/user';

const User = sequelize.import('users', function(sequelize, DataTypes) {
  return new userModel(sequelize, Sequelize.DataTypes);
})

[felixfbecker]

@OLDIN that is completely wrong and has nothing to do with the new syntax. A constructor does not have a return value.

This is a very old PR. If you want to discuss anything, please move the discussion to Slack, Gitter or a new issue.

[OLDIN]

@felixfbecker offer your working version. I will be glad to use.

[felixfbecker]

@OLDIN I can point you to the TypeScript definitions, which contain some examples: http://typed-sequelize.surge.sh/

I'm gonna lock this thread now to prevent spamming participants with notifications