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

4.0 Model definition syntax #6524

Closed
felixfbecker opened this Issue Aug 31, 2016 · 83 comments

Comments

@felixfbecker
Contributor

felixfbecker commented Aug 31, 2016

Because the dicussion is spread across many issues and PRs, I wanted to open a new issue to discuss the future syntax for model definition.

Currently, besides the old define(), this syntax is possible in v4.0.0-1:

export class User extends Model {
 
    get fullName() {
        return this.firstName + ' ' + this.lastName;
    }
 
    set fullName(fullName) {
        const names = fullName.split(' ');
        this.lastName = names.pop();
        this.firstName = names.join(' ');
    }
}
User.init(({
    username: {
        type: DataTypes.STRING,
        primaryKey: true
    },
    lastName: DataTypes.STRING,
    firstName: DataTypes.STRING,
}, { sequelize })

If you prefer to not do the initialization (which also attaches the connection to the model) inside the model file, but rather from a model index file, this is also possible:

export class User extends Model {
 
    get fullName() {
        return this.firstName + ' ' + this.lastName;
    }
 
    set fullName(fullName) {
        const names = fullName.split(' ');
        this.lastName = names.pop();
        this.firstName = names.join(' ');
    }

    static init(sequelize) {
        super.init({
            username: {
                type: DataTypes.STRING,
                primaryKey: true
            },
            lastName: DataTypes.STRING,
            firstName: DataTypes.STRING,
        }, { sequelize })
    }
}

With the help of a decorator library and Babel transpilation, this is also possible:

@Options({ sequelize })
@Attributes({
    username: {
        type: DataTypes.STRING,
        primaryKey: true
    },
    lastName: DataTypes.STRING,
    firstName: DataTypes.STRING,
})
export class User extends Model {
 
    get fullName() {
        return this.firstName + ' ' + this.lastName;
    }
 
    set fullName(fullName) {
        const names = fullName.split(' ');
        this.lastName = names.pop();
        this.firstName = names.join(' ');
    }
}

And with TypeScript you can go even further:

@Options({ sequelize })
export class User extends Model {
 
    @Attribute({
        type: DataTypes.STRING,
        primaryKey: true
    })
    public usernamestring;
 
    @Attribute(DataTypes.STRING)
    public firstNamestring;
 
    @Attribute() // Type is inferred as DataTypes.STRING 
    public lastNamestring;
 
    get fullName(): string {
        return this.firstName + ' ' + this.lastName;
    }
 
    set fullName(fullNamestring) {
        const names = fullName.split(' ');
        this.lastName = names.pop();
        this.firstName = names.join(' ');
    }
}

BUT we definitely do not want to require the usage of any transpiler. The Model.init() API feels a bit weird, especially how the sequelize connection is passed. Some thoughts:

  • If we want to make models independent one day from connections, this is has the benefit of easily omitting the sequelize option
  • But we would probably still need to require that models are registered in a model manager, so we could inverse the syntax (something like sequelize.add(Model) instead of passing the connection. That is not implemented atm though, currently the model needs to know about the connection.

Please chime in and share your thoughts and ideas!

Issues/PRs for reference: #5898 #6439 #5877 #5924 #5205 #4728

@felixfbecker felixfbecker added this to the 4.0 milestone Aug 31, 2016

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Aug 31, 2016

@leebenson

The only small addition I'd love to see is a way to get the attributes defined in the class, similar to the syntax I referenced in #6439 (comment)

@options({ tableName: 'user', sequelize }) class User extends Model {

 // Attributes can be specified by a decorator...
@attribute name = {
   type: DataTypes.STRING,
   unique: true,
   // ...
 }

 // Class methods can be created using the `static` keyword
 static someClassMethod() {
   // `this` references the loaded sequelize instance... so we can get other models, etc.
 }

 // And instance methods are everything non-static
 someInstanceMethod() {
   // this.name, etc
 }
}

That's how Mobx do it, and adoption seems to be pretty good. Personally, I find the more that can be described succinctly in the body of the class, the easier it is to reason about what's a field, what's a class method and what's an instance, without having to define those in multiple places.

So the attribute configuration is defined as an attribute name on the model prototype? Is that even supported syntax in Node 4/6? Is that the attribute initializer proposal? If yes, I don't think we should put the configuration directly on the prototype per-attribute. It should sit under attributes, as it does now. And we shouldn't try to make JavaScript look like TypeScript by abusing property initializers.

@leebenson

This comment has been minimized.

leebenson commented Aug 31, 2016

The decorator proposal is in flux, so I can't say whether that will be a future ECMAScript standard. That's basically what mobx do right now, and it works well.

In the meantime, this is how I'm handling loading models in v4. I'd appreciate your advice if you think this abuses the current API in any way.

Model loader

(influenced by Sequelize CLI 2.3; updated to match my environment)

// Checks whether we have the right env vars set; exits if not
require('./env')('DB_NAME', 'DB_USER', 'DB_PASSWORD');

import fs from 'fs';
import path from 'path';
import Sequelize from 'sequelize';

// Delay returns a promise that resolves after the number of
// milliseconds passed have elapsed.  This is useful for 'suspending'
// an awaited statement inside of an async function (in other words -
// to simulate a pause)
import delay from 'delay';

// Path to look for model files
const modelPath = path.join(__dirname, 'db_models');

// Open a new Sequelize client
const sequelize = new Sequelize(
  process.env.DB_NAME, // database name
  process.env.DB_USER, // username
  process.env.DB_PASSWORD, // password
  {
    host: 'percona', // points to a linked Docker host
    dialect: 'mysql',
  },
);

// Load each model file
export const models = Object.assign({}, ...fs.readdirSync(modelPath)
  .filter(function (file) {
    return (file.indexOf('.') !== 0) && (file.slice(-3) === '.js');
  })
  .map(function (file) {
    const model = require(path.join(modelPath, file)).default;
    return {
      [model.name]: model.init(model.fields, {
        sequelize,
      }),
    };
  })
);

// Load model associations
for (const model of Object.keys(models)) {
  typeof models[model].associate === 'function' && models[model].associate(models);
}

// Ping checks that the database server can be connected to. It attempts
// to connect every second for 60 seconds, before giving up and throwing
// an error back up to the chain.  This function is especially useful when using
// Docker Compose with a linked DB, which may not always be ready before
// the API server starts
export async function ping() {
  for (let i = 1; i <= 60; i++) {
    try {
      await sequelize.authenticate();
      return;
    } catch (e) {
      console.log(`DB connection attempt #${i} failed, retrying...`);
      await delay(1000);
    }
  }

  throw new Error("Couldn't connect to database!");
}

Sample Model definition (in this case, 'Session')

export default class Session extends Model {

  static fields = {
    id: {
      type: Sequelize.UUID,
      defaultValue: Sequelize.UUIDV4,
      primaryKey: true,
    },

    browser: {
      type: Sequelize.TEXT,
    },

    expiresAt: {
      type: Sequelize.DATE,
      defaultValue() {
        return moment().add(30, 'days').toDate();
      },
    },

    whatever: {
      type: Sequelize.VIRTUAL,
      get() {
        return 'it worked!';
      },
    },

    idAgain: {
      type: Sequelize.VIRTUAL,
      get() {
        return this.get('id');
      },
    }
  };

  // Session middleware that Koa uses to create a `ctx.session` object
  // for all subsequent middleware. (method appears at class-level and
  // *not* on the instance

  static middleware() {

    // We'll return this as an async function, so that it can be invoked
    // in the Koa config and bound to the currently loaded model
    return async function (ctx, next) {
      // ... omitted for brevity
    }.bind(this);
  }

  // This function is instance-level... it'll bind to the active record
  instanceFunction() {
    return true;
  }
}

Invoking the above inside an async function seems to work perfectly...

import * as db from './dbLoader';

(async function () {
  // Wait for the DB to be ready
  await db.ping();

  // Start a new instance
  const s = new db.models.Session;

  // Make sure we've got what we expect...
  console.log('s.id ->', s.id);
  console.log('s.idAgain ->', s.idAgain);
  console.log('s.middleware -> ', s.middleware);
  console.log('s.fields ->', s.fields);
  console.log('s.whatever ->', s.whatever);
  console.log('s.instanceFunction() ->', s.instanceFunction());

  // Plus the static method means I can wire up things that belong at
  // the class-level, like middleware
  const app = new Koa;
  app.use(db.models.Session.middleware());
})();

... which returns the expected output:

server_1 | Executing (default): SELECT 1+1 AS result
server_1 | s.id -> 5c3f792a-c66f-442d-96f8-ccbb630f7bae
server_1 | s.idAgain -> 5c3f792a-c66f-442d-96f8-ccbb630f7bae
server_1 | s.middleware -> undefined
server_1 | s.fields -> undefined
server_1 | s.whatever -> it worked!
server_1 | s.instanceFunction() -> true

@leebenson

This comment has been minimized.

leebenson commented Aug 31, 2016

A few reasons why I chose the syntax in the above post.

  • IMO, decorators proved to be overkill; assigning a static fields object at the class level gives me the 'class encapsulation' I was looking for without relying on an ECMAScript proposal that's in flux.
  • Defining static/instance methods in one place and fields in another looked disjointed... this solves it by placing field defs inside the class, without bleeding out to the instance.
  • The model loader wires up the models, so all we need to care about is creating a class that extends Sequelize.Model and exporting it as the default. This is a good separation of concerns, I think - previously, I was doing the sequelize.define() separately inside of each model file and wound up with repeated boilerplate for every new table definition.
  • Table names are implicitly added from the class name, reducing repetition with an explicit tableName property.
@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Sep 1, 2016

@leebenson

IMO, decorators proved to be overkill; assigning a static fields object at the class level gives me the 'class encapsulation' I was looking for without relying on an ECMAScript proposal that's in flux.

See my previous comment. Yes, decorators are still a proposal, but the complete Angular 2 framework is already built on it and that is now at a release candidate, people already use this in production and it works very well.

Please note that class properties can not be used in Node 4/6. It is a Stage 1 proposal, while decorators are already Stage 2. So the API you are proposing will not work without transpilation either, just like a decorator API.

I think aestetically, a static property and a decorator are on the same level, but decorators fit the use case better semantically (you decorate the class with metadata) and they have the benefit of normalization, while the class property would result in unexpected behaviour and would still require a seperate .init() call.

Defining static/instance methods in one place and fields in another looked disjointed

I agree.

Table names are implicitly added from the class name, reducing repetition with an explicit tableName property.

This is already implemented in 4.0.0-1. The tableName was just an example of passing options, I removed it to clarify it.

The model loader wires up the models, so all we need to care about is creating a class that extends Sequelize.Model and exporting it as the default. This is a good separation of concerns, I think - previously, I was doing the sequelize.define() separately inside of each model file and wound up with repeated boilerplate for every new table definition.

I don't get what you are trying to say. Imo a model file should be as self-contained as possible. I actually even use late imports instead of the module.exports = function(sequelize, DataTypes) { pattern, so I don't even need a model index file that "wires up" the models, they wire up themselves, including associations. I tried to highlight a bit of this in my PR to update the docs, see https://github.com/felixfbecker/sequelize/blob/76590ab17ad8d160272a7e02bd7b381bc49ae919/docs/docs/models-definition.md#one-file-per-model-pattern

@leebenson

This comment has been minimized.

leebenson commented Sep 1, 2016

See my previous comment. Yes, decorators are still a proposal, but the complete Angular 2 framework is already built on it and that is now at a release candidate, people already use this in production and it works very well.

No arguments there. I'm already using decorators with mobx too, and I love them! I just couldn't find a better fit for wiring up field definitions than a static property on the class. A decorator function seemed like overkill.

I don't get what you are trying to say. Imo a model file should be as self-contained as possible. I actually even use late imports instead of the module.exports = function(sequelize, DataTypes) { pattern, so I don't even need a model index file that "wires up" the models, they wire up themselves, including associations. I tried to highlight a bit of this in my PR to update the docs, see https://github.com/felixfbecker/sequelize/blob/76590ab17ad8d160272a7e02bd7b381bc49ae919/docs/docs/models-definition.md#one-file-per-model-pattern

What I'm saying is that defining field types within the body of the class seems like a better approach.

In your example, you do this...

class Pub extends Sequelize.Model { }
Pub.init({
  name: Sequelize.STRING,
  address: Sequelize.STRING,
  latitude: {
    type: Sequelize.INTEGER,
    allowNull: true,
    defaultValue: null,
    validate: { min: -90, max: 90 }
  },
  longitude: {
    type: Sequelize.INTEGER,
    allowNull: true,
    defaultValue: null,
    validate: { min: -180, max: 180 }
  }
}, {
  sequelize,
  validate: {
    bothCoordsOrNone() {
      if ((this.latitude === null) !== (this.longitude === null)) {
        throw new Error('Require either both latitude and longitude or neither')
      }
    }
  }
})

This part irks me...

class Pub extends Sequelize.Model { }
Pub.init({
// ...
})

You've created a class to extend Model, but it's blank. Presumably the only benefit here is that you've removed the need to explicitly define tableName.

Then in a separate call, you initialise it with .init() and feed in the field definitions.

IMO, it looks better to keep them defined within the class. Whether it's a decorator or a static property is less relevant... more so is that syntax outside of the class makes it hard to reason about exactly what fields the model contains. I have 500+ lines of field definitions and boilerplate in some places... I'd rather keep that inside the class than stick it where my .init() code goes, which could well be somewhere outside of the model definition entirely.

Just my personal preference.

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Sep 1, 2016

Yes, init() after the class is definitely the ugly part about the API. But as I said, there is no way to do it without using a transpiler atm (and then decorators are a better solution). And we of course also still have define().

@leebenson

This comment has been minimized.

leebenson commented Sep 1, 2016

Another option is static method... they've been in Node since 4+ and don't require any transpilation at all:

class Session extends Sequelize.Model {
  static fields() {
    return {
      id: {
        type: Sequelize.UUID,
        // ....
      }
    }
  }
}
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 1, 2016

@felixfbecker Is the model name taken from the class name? In that case we need a way to override it because a lot of people generally have lower case model names but would probably have lint rules telling them to have uppercase class names

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Sep 1, 2016

@leebenson Yes, but imo having a method that always returns the same configuration object just because the language doesn't have property declarations yet is syntax abuse and DSLish.

@mickhansen yes, the model name is the name property of the class, which is the class name. It is a native property of all functions. It can be overridden with the modelName option or when using sequelize.define(). But to be honest I think we should use PascalCase naming in the docs because it is confusing when User.name !== 'User'.

@leebenson

This comment has been minimized.

leebenson commented Sep 1, 2016

@felixfbecker re: lower casing, I'm currently doing it explicitly like this (in my model loader)

import utils from 'sequelize/lib/utils';

// Load each model file
export const models = Object.assign({}, ...fs.readdirSync(modelPath)
  .filter(function (file) {
    return (file.indexOf('.') !== 0) && (file.slice(-3) === '.js');
  })
  .map(function (file) {
    const model = require(path.join(modelPath, file)).default;
    return {
      [model.name]: model.init(
        (typeof model.fields === 'function' && model.fields()) || {},
        Object.assign({
            sequelize,
            tableName: utils.pluralize(model.name.toLowerCase()), // <--- this bit
          },
          (typeof model.attributes === 'function' && model.attributes()) || {},
        ),
      ),
    };
  })

With my session definitions now looking closer to this:

export default class Session extends Model {

  static fields() {
    return {
      id: {
        type: Sequelize.UUID,
        defaultValue: Sequelize.UUIDV4,
        primaryKey: true,
      },
     // ....
    };
  }

//.....
}

So a Session class becomes a sessions table.

I agree with you that a method returning the same static data isn't ideal. It's an alternative to requiring a transpilation step, though, and it's only run once. Plus, it's an opportunity to bind the current sequelize client as a parameter, in case definitions somehow rely on it.

Hard to get that with a static class attribute or a decorator that's outside of the definition scope.

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Sep 1, 2016

Please note that tableName and modelName are two different options.

Plus, it's an opportunity to bind the current sequelize client as a parameter, in case definitions somehow rely on it.

I am highly against this. In a static method, I expect this to be the class. Or are you not talking about binding?

Hard to get that with a static class attribute or a decorator that's outside of the definition scope.

It's super simple to share the sequelize instance is to simply have a module that exports it, and require it in the model files.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 1, 2016

But to be honest I think we should use PascalCase naming in the docs because it is confusing when User.name !== 'User'

Definitely, just need to make sure it's possible to reconfigure

@leebenson

This comment has been minimized.

leebenson commented Sep 1, 2016

Please note that tableName and modelName are two different options.

I know, but in the absence of either being set explicitly, the pluralised version of class.name is used, right?

I think this is what @mickhansen was referring to. It threw me off too. Hence the pluralize hack above. Model.init ought to lowercase the class name before it implicitly pluralises it for use as a model/table name.

I am highly against this. In a static method, I expect this to be the class. Or are you not talking about binding? The best pattern to share the sequelize instance is to simply have a module that exports it, and require it in the model files.

Binding as a parameter. Not to this.

So you can do something like...

class Session extends Sequelize.Model {
  static fields(sequelize, DataTypes) {
    return {
      id: {
        type: DataTypes.UUID,
        // ....
      }
    }
  }
}

I do use this elsewhere on my class, btw, for things like GraphQL...

class Session extends Sequelize.Model {

  // GraphQL public mutations
  static mutations(db, types) {

    // models = access to `db`, which contains db.models.*, db.sequelize, etc
    // types = GraphQL types, defined outside of this class

    return {
      logout: {
        type: types.Session,
        async resolve(root, args, ctx) {
          // ctx = current Koa context for the web user

          await ctx.session.logout();
          return ctx.session;
        },
      },
    };
  }
}

It keeps model logic neatly in one place, without polluting any model instance.

@leebenson

This comment has been minimized.

leebenson commented Sep 1, 2016

Actually, I've just realised that's exactly what you're already doing here with associate(), so wouldn't having an 'official' fields() and attributes() static method along with a standardised model loader (like you have in the CLI, and I modified above) be a decent solution overall?

As much as I love decorators, I can't help but feel they're overkill in this scenario because they require explicit access to sequelize to work, which might not even be defined inside the class file (it isn't for me.)

An example like this could just as easily be...

export class User extends Model {

  // For field definitions
  static fields(sequelize, DataTypes) {
    username: {
        type: DataTypes.STRING,
        primaryKey: true,
    },
    lastName: DataTypes.STRING,
    firstName: DataTypes.STRING,
  }

  // For options
  static options(sequelize) {
    return {
      tableName: 'someTotallyDifferentName',
    };
  }

  // For asociations
  static associate(sequelize) {
    this.belongsTo(sequelize.Session);
  }

  // Instance-level methods...
  get fullName() {
      return this.firstName + ' ' + this.lastName;
  }

  set fullName(fullName) {
      const names = fullName.split(' ');
      this.lastName = names.pop();
      this.firstName = names.join(' ');
  }
}

... with:

  • No transpilation step (works out the box in Node 4+)
  • No need to worry about scope... sequelize and DataTypes can be fed in from the 'official' model loader with no external dependencies
  • Syntax that already works in the same way as the CLI uses associate()
  • No bleeding out to the instance - new User doesn't see or care about them
@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Sep 1, 2016

In your example, options() doesn't set the sequelize instance. The model needs to know the connection, it is a required option.

Please note that associate() isn't anything magic, it is just a static method that needs to be called manually in a model index file. And in my opinion in the whole way the CLI promotes it it is a terrible pattern. When you import or require a module, your IDE knows the type of the module. But it is not explicit when they are simply passed as arguments. Your IDE cannot know what the type of sequelize.User is, or even if it exists. You could have a typo and would not get any help. It also cannot resolve a model that is imported with sequelize.import() to a type. Why don't you simply use import / require?
db.js

import {Sequelize} from 'sequelize'
export default sequelize = new Sequelize(process.env.DB)

User.js

import {DataTypes} from 'sequelize'
import sequelize from '../db'
export class User {
  ...
}
User.init({ ... }, { sequelize })

import UserGroup from './UserGroup'
User.belongsTo(UserGroup)

UserGroup.js

import {DataTypes} from 'sequelize'
import sequelize from '../db'
export class UserGroup {
  ...
}
UserGroup.init({ ... }, { sequelize })

import User from './User'
UserGroup.hasMany(UserGroup)

We save the attributes under attributes and options under options. We should not name the static methods that way to, not fields. But you cannot have a method that has the same name as an attribute, it would get overridden, and that is really weird if first have attributes be a function and later it is an object.

Regarding decorators being overkill: We need to normalize attributes and options. For example, Sequelize.STRING needs to be converted to new Sequelize.STRING(), defaults are applied. To support inheritance, we will let attributes and options inherit prototypically from the parent model's options/attributes. We could do all this on every query, but that would be overhead. We need to do it on normalization, so that all successive calls can rely on a structure they can easily work with. And an API should have the meet expectations of the user:

  • if I provide a static property options, I expect it to stay as I provided it. If I change it later on, I expect these changes to be taken into account
  • if I provide a static method options(), I expect it to stay a method, and be the single source of truth for the options. A method to me suggests that it could return dynamic content, it could change on successive calls (which is a false impression)

Decorators and init() don't face this problem. You input your options, and they normalize them, apply defaults, and then assign them to options.

Having these static methods return configuration is what put me and a lot of developers I talked to off from many of the PHP ORMs. In PHP you have no other option because patterns like a class factory are not possible, but in JavaScript I would prefer the define() API over any static method API. We have to keep in mind that one day, these proposals will make it into JS. They are used by people with transpilers. And for plain Node 4/6 environments users can choose if they want to use define() or classes with a init() call.

@leebenson

This comment has been minimized.

leebenson commented Sep 1, 2016

I understand that your example above is the way it works now. I provided alternate syntax because it doesn't seem like you're settled a pattern yet, and IMO, my way was cleaner.

FWIW, here's a more complete example of what my User model currently looks like (in production; cut for brevity)

import Sequelize, { Model } from 'sequelize';

export default class User extends Model {

  /* FIELDS */

  static fields() {
    return {
      id: {
        type: Sequelize.UUID,
        defaultValue: Sequelize.UUIDV4,
        primaryKey: true,
        allowNull: false,
      },
      email: {
        type: Sequelize.STRING,
        allowNull: false,
      },
      password: {
        type: Sequelize.STRING,
        allowNull: false,
      },
      firstName: {
        type: Sequelize.STRING,
        allowNull: false,
      },
      lastName: {
        type: Sequelize.STRING,
        allowNull: false,
      },
    };
  }

  /* RELATIONSHIPS */

  static associate(models) {
    this.hasMany(models.Session);
  }

  /* CLASS-LEVEL FUNCTIONS */

  // Create a new user
  static create(args) {
    // logic to create a user
  }

  /* GRAPHQL MUTATIONS */
  static mutations(g, t) {
    return {

      // Create a new user
      createUser: {
        type: t.User,
        args: {
          email: {
            type: g.GraphQLString,
          },
          firstName: {
            type: g.GraphQLString,
          },
          lastName: {
            type: g.GraphQLString,
          },
          password: {
            type: g.GraphQLString,
          },
        },
        async resolve(root, args) {
          return create(args);
        },
      },
    };
  }
}

There are a few things here, IMO, that are improved over the syntax I've seen so far:

  1. Few dependencies. Importing Sequelize + Model is all I need.
  2. I don't need to worry about an instantiated sequelize object or pass it around models. That's defined in another file, where I also have a 'model manager' that pulls in the models. IMO, models should be loaded into the connection... not the other way around. Otherwise every model winds up importing the same object and repeating the boilerplate.
  3. The associate static (pulled from the CLI) is actually one of the best parts of the model loader, IMO. It ensures that Model A can always reference Model B, and it neatly avoids circular dependencies. In your example above, if User.belongsTo(UserGroup) and UserGroup.hasMany(User), you're relying on both model files being fully instantiated before exporting and importing into sister models. It's neater to define this behaviour in a model manager when you're already working with loaded data.
  4. It fills well in wider ecosystems. My model classes aren't 'just' for Sequelize; they group together related logic for other parts of my app (in the above example, GraphQL). So having a common DI pattern where variables are passed to static fields is an easy way to reason about the purpose of a model class. Having fields() and attributes() satisfies an interface-like pattern for Sequelize, the same way mutations() does for GraphQL.
  5. It works out the box. That's not a big deal for me (my webpack build is 500+ lines long, and I'm transpiling pieces for both the server and the front-end from one universal repo - so I'm used to designing for polyfills), but for other users following along with your documentation, it would be ideal to show a really easy way to define and manage models that's as simple as something like this...

user.js

import { Model } from 'sequelize';

export class User extends Model {
  static fields() {
    // return field def...
  }

  static options() {
    // return options...
  }

  fullName() {
    return `${this.firstName} ${this.lastName}`;
  }
}

db.js

import Sequelize from 'sequelize';
import UserModel from './user'

export const connection = new Sequelize(/* connection vars */);
connection.addModel(UserModel);

app.js

import { connection } from './db';

connection.models.User.findOne(/* search args */).then(user => {
  // user = found user
});

Instead, a user has to:

  1. Use TypeScript, or fiddle with .babelrc to get the right combination of plug-ins (if they're following the decorate route).
  2. Write classes empty classes and then use a slightly weird class.init() initialisation, that takes two parameters defining field types and a sequelize object- both completely disconnected from the class that's being initialised OR...
  3. Use decorators, which are actually exactly the same thing, but just re-shuffled to appear before the class rather than after it.
  4. Figure out a way for models to play well with one another, whilst avoiding circular dependencies and juggling multiple imports.
  5. Remember parameter order and do things like pass { sequelize } in the 'options' object, which a model manager could inject behind the scenes.

I'm just providing friendly feedback, as a user of Sequelize since the very early days. In v4 you have a chance to drastically reduce boilerplate. I think what you currently have just adds noise. Why is it any better than defining instanceMethods or classMethods on a giant object to define()?

Again, I'm brand new to v4, so maybe you've figured those kinds of things out and I'm way off the mark. Maybe you already have a model manager that wires stuff up. I'm still figuring out my way around v4.

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Sep 1, 2016

@leebenson I really appreciate your feedback, please don't misunderstand my critic. 🙂 You bring in interesting ideas and it's nice to hear from someone who actually uses v4 with ES6 syntax. I hope I didn't come up rude while being open with my thoughts 😅 Long post incoming.

Few dependencies. Importing Sequelize + Model is all I need.

Yes, you have an inversion of control. But your model is still coupled (dependent) on the sequelize instance. You just inverse the control over it.

IMO, models should be loaded into the connection... not the other way around

Yes, that would be my dream API too. It would be nice if models did not know about their connection at all, you only add them to a model manager and either pass the connection to use to find() etc or optionally set the default connection. This was kind of the idea behind having sequelize as an attribute of option, it should be the "default" connection and one day we allow to omit it.

Your API definitely is a viable workaround, but please not ethat there are some things which don't make it suitable for the API:

  • You name the function fields, but later they will be saved under attributes. In Sequelize (docs), the term attribute generally refers to the model attributes, while field refers to the database column (see the fieldName option for example). It is fine for your workaround, but for our future API, for consistency, the method would have to be named attributes. But the property is already named attributes, and there are use cases for reading the (normalized) attributes at runtime.
  • In the same way, your example with static options() actually has exactly this problem. As soon as you execute init(). the method will get overridden with the options hash. And that is very unexpected behavior.

Whatever API we settle on, I would like it to be forward compatible. It should be built in mind with JS features that will arrive in the future. For example, with the decorator API, users who use decorators now can do it, users who don't can use .init() (decorators use init under the hood). Once they become broadly available, they can switch to that syntax, but we don't have to deprecate anything. But when we abuse static methods now, that API will become obsolete once we have class properties in JS, and even worse, directly conflict in terms of naming and it would be a breaking change to allow the usage of class properties.

The associate static (pulled from the CLI) is actually one of the best parts of the model loader, IMO. It ensures that Model A can always reference Model B, and it neatly avoids circular dependencies. In your example above, if User.belongsTo(UserGroup) and UserGroup.hasMany(User), you're relying on both model files being fully instantiated before exporting and importing into sister models. It's neater to define this behaviour in a model manager when you're already working with loaded data.

I don't know what you mean with "you're relying on both model files being fully instantiated before exporting". The idea of a late import is based on the fact that the module is not complete yet, NodeJS has excellent support for this, even a dedicated documentation section highlighting this feature. In short:

  • User module exports User class
  • User module requires UserGroup module
  • UserGroup module exports UserGroup class
  • UserGroup requires User module and gets the already exported User class
  • UserGroup module associates UserGroup to User
  • User module gets the UserGroup class and associates User to UserGroup

I look at it this way:
Through the imports, it is made clear that the modules depend on each other (they do, no matter how you put it). An IDE can reason about the required type.
When using the alternative of injecting models or sequelize as a parameter, and then access your associated models from there, there is no contract ensuring that the models you need actually exist on that object. There isn't even a contract saying that associate will be called at all. It is a loose contract, the callee has to make sure that he first imports all the other models that the model may need before calling associate.
I have a projects with close to 100 model files, and from my experience this pattern doesn't scale. We have isolated unit and integration tests that only test small parts of the app, for example the custom methods on a single model or an express router that handles a specific group of endpoints regarding a model. With the import pattern, I don't need to require an index file. The router only imports the model it uses, and the model in turn will import only the models it is associated with plus the module exporting the connection. This means you can run such a test easily in isolation, and it will not touch the other 95 models at all. It is 100% clear which models depend on each other - for the developer, for the IDE, for the test runner, for the coverage tool.

So I don't even try to avoid the circular dependency between model A and B, I embrace it. It is one by definition, because it is a relation, and a relation is symmetrical.

This pattern of exporting a closure that is called with sequelize reminds me a lot of the way we structured express apps back in the day before there was a composable Router object. We had to export a closure, that then got called with the express app and patched it routes it. Imo the more we can move away from exporting functions that need to be called by the requirer the better.

but for other users following along with your documentation, it would be ideal to show a really easy way to define and manage models that's as simple as something like this...

As it stands right now, I guess we will likely keep the examples in the docs with define() until we have some more JS features broadly available.

Why is it any better than defining instanceMethods or classMethods on a giant object to define()?

You hit the nail on the head here unfortunately. Currently none of the syntaxes that work with native Node 4/6 are a great improvement over define(). It's nice that people who have latest JS features available can use the new syntax now (including you, if you want to use static methods), but personally I'm now leaning more towards keeping docs with define() and not introduce any new syntax that uses methods not because it makes sense, but just for the sake of having it inside the class body.

I'm transpiling pieces for both the server and the front-end from one universal repo

Out of interest, what do you mean by this? Are you using Sequelize in some way for an isomorphic app?

@leebenson

This comment has been minimized.

leebenson commented Sep 1, 2016

@leebenson I really appreciate your feedback, please don't misunderstand my critic. 🙂 You bring in interesting ideas and it's nice to hear from someone who actually uses v4 with ES6 syntax. I hope I didn't come up rude while being open with my thoughts

Thanks!

Now here comes an equally long post, I'm afraid... 🙂

Few dependencies. Importing Sequelize + Model is all I need.

Yes, you have an inversion of control. But your model is still coupled (dependent) on the sequelize instance. You just inverse the control over it.

Yup, it's still tightly coupled (the model and the connection have to meet at some point, after all), but I think it'd be useful for that logic to happen behind the scenes for most users.

If...

const sequelize = new Sequelize(/* connection */);

... contains the connection, then feeding models into it with something like sequelize.addModel(ModelClass) feels like a solid design choice. Having to force it onto models means taking a model manager that could occur in one place, and adding boilerplate to every model. Not an issue for a single model... but at scale, it starts to add up.

Your API definitely is a viable workaround, but please not ethat there are some things which don't make it suitable for the API:

  • You name the function fields, but later they will be saved under attributes. In Sequelize (docs), the term attribute generally refers to the model attributes, while field refers to the database column (see the fieldName option for example). It is fine for your workaround, but for our future API, for consistency, the method would have to be named attributes. But the property is already named attributes, and there are use cases for reading the (normalized) attributes at runtime.
  • In the same way, your example with static options() actually has exactly this problem. As soon as you execute init(). the method will get overridden with the options hash. And that is very unexpected behavior.

Understood. I haven't poked around the internals, so I don't know how you're currently mutating underlying classes/models to reflect options, or what you consider 'reserved words'.

No doubt, my example can't be used literally. But the fact that there's namespace clashes is sort of my point. IMO, it's a better design choice if:

  1. The model is 'bare'. It contains just enough to define itself.
  2. There's no need to worry about connection objects - the model loader/manager should handle it. Why pass around sequelize to 50 models that require 50 sets of boilerplate, when you can just import them through an .addModel()-esque method?
  3. .addModel() automatically creates a "higher order" component where fields, options, attributes (or whatever else you internally designate them to be) are outside of the model class. This could be done well with something like ES6's WeakMap where the class is the key, and the data contains the model config pulled from it. No need to mutate state.

Whatever API we settle on, I would like it to be forward compatible. It should be built in mind with JS features that will arrive in the future. For example, with the decorator API, users who use decorators now can do it, users who don't can use .init() (decorators use init under the hood). Once they become broadly available, they can switch to that syntax, but we don't have to deprecate anything. But when we abuse static methods now, that API will become obsolete once we have class properties in JS, and even worse, directly conflict in terms of naming and it would be a breaking change to allow the usage of class properties.

I totally understand, but I would counter in two ways:

  1. Static methods aren't being 'abused' if they serve a purpose. .addModel() could pass a Sequelize/DataTypes object to it to reduce dependencies in the model class for types such as Sequelize.STRING, or to gain access to the raw sequelize connection object. You can't do that with decorators or static class attributes - in both cases, you'd need to know those variables ahead of time. With static methods, we inherit the benefit of late parameter binding.
  2. Decorators will almost certainly remain (in some form), but they're in flux. Babel has this to say about them:

Decorators are still only a relatively new proposal, and they are (at least currently) still in flux. Many people have started to use them in their original form, where each decorator is essentially a function of the form

function(target, property, descriptor){}

This form is very likely to change moving forward, and Babel 6 did not wish to support the older form when it was known that it would change in the future. As such, I created this plugin to help people transition to Babel 6 without requiring them to drop their decorators or requiring them to wait for the new proposal update and then update all their code.

My point is not that decorators are bad (they're not), just that relying on them to work in a certain way is shaky ground. Plus, are they really needed? Everything you can do with decorators, you can do just as easily with static methods and a good .addModel() implementation that hides the instantiation details.

mobx make great use of decorators, and I'd argue their use are worth taking a meantime punt on makeshift syntax. However, I'd also argue they serve a valid purpose - using an @observer decorator directly manipulates the third parameter in the current decorator implementation (descriptor) by changing property attributes on the underlying object and adding observers. By contrast, Sequelize isn't really doing anything interesting enough with them... it's just a way to define meta data in one place rather than another.

There's definitely ways this could be made more interesting, I think. I use decorators in my own projects to provide "higher order" functions in React, mobx, etc. My decorators usually have bound 'state', so when I do something like this in my React classes...

@connect class Button extends React.Component {
  render() {
    // @connect has provided this function with `this.context.state`...
  }
}

... then @connect is more than just sugar - it's implicit state.

I apologise if I haven't understand your current decorator implementation correctly, but it seems if they require an explicit sequelize object, they're not really serving the same purpose.

I don't know what you mean with "you're relying on both model files being fully instantiated before exporting". The idea of a late import is based on the fact that the module is not complete yet, NodeJS has excellent support for this, even a dedicated documentation section highlighting this feature. In short:

Node has great support, but I've always thought of circular dependencies as an anti-pattern.

Maybe this stems from my work with Go or other languages, but I'd generally agree with the points in the above link for any language, for the same reasons.

So I don't even try to avoid the circular dependency between model A and B, I embrace it. It is one by definition, because it is a relation, and a relation is symmetrical.

If models rely on each other, I'd think it's better design to import them from a third, 'impartial' class (i.e. the sequelize definition and model loader/manager) and handle relationship connections post-import. Incidentally, the CLI already works that way (possibly just as a convenience, but the end result is the same.) Again, a decent model loader could easily track dependencies and wire them up as they become available. WeakMap could be a good candidate for this, too.

Why is it any better than defining instanceMethods or classMethods on a giant object to define()?

You hit the nail on the head here unfortunately. Currently none of the syntaxes that work with native Node 4/6 are a great improvement over define(). It's nice that people who have latest JS features available can use the new syntax now (including you, if you want to use static methods), but personally I'm now leaning more towards keeping docs with define() and not introduce any new syntax that uses methods not because it makes sense, but just for the sake of having it inside the class body.

I think that's smart, but I also think alternatives exist today that can achieve the effects of decorators, allow model classes to be 'bare' per my criteria list above, and define models inside a connection without passing around sequelize. I do exactly the same thing with React. You won't find explicit state anywhere in my call stack. Even per-request state is injected right through without affecting static compilation of my components/models. When creating universal apps, it's sort of a requirement... I'd love to see this same kind of design in Sequelize.

I'm transpiling pieces for both the server and the front-end from one universal repo

Out of interest, what do you mean by this? Are you using Sequelize in some way for an isomorphic app?

Yes. In one project, I have a common code base that employs:

  • React for views (rendered as HTML on the server, and as 'single page' in the browser)
  • Mobx for state, sent down the wire to the client
  • Isomorphic fetch, using the server as a proxy to bypass CORS
  • Sequelize behind GraphQL, that can be called from either environment
  • CSS/asset loading using inline require() calls, in both environments

I use webpack to create three separate bundles:

  • Development, using hot-code-reload. Change anything, and it's pushed to the browser. Source maps, assets pushed down the wire, etc.
  • Production: client side. Assets minified and hashed. SASS autoprefixes. Client side bundle that routes all external requests through a server proxy, but is otherwise the same syntax.
  • Production: server-side. A Node-friendly bundle that parses out inline CSS/assets, adds polyfills for missing ES6 features, implements fetch calls in a server compliant way, etc.

Sequelize is my main ORM in most projects, and this one too.

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Sep 1, 2016

Response will come tomorrow, but one thing just came to my mind:
You don't like init() because it comes after the class declaration, and you prefer to have a static method that "wires up" the model, that then is called by your model index file. You want to inject connection as a parameter. What do you think about this?

class User extends Model {
  static init(sequelize) {
    super.init({
      // attributes
    }, {
      sequelize,
      // options
    })
  }
}

You can call this from your model index file, you have the declarations in the class, you dont run into any name collisions, and it is supported by 4.0.0-1.

@leebenson

This comment has been minimized.

leebenson commented Sep 2, 2016

@felixfbecker that's a decent approach. It keeps things in one place without name clashes. Nicely done. I'll probably go for this approach instead of having multiple methods, at least until I know more about the internals not to inadvertently overwrite reserved words.

I still think there's some interesting things that could be done to the model manager that won't require connection strings or special boilerplate. I think that would open up your use of decorators, too. Something along these lines:

_Session.js model_

import { model, options, relationships }, DataTypes from 'sequelize'

@model({ id: DataTypes.STRING })
@options({ tableName: 'somethingElse'})
@relationships({ belongsTo: ['User'], hasMany: ['SessionData']})
export default class Session {

  // Anything on the class = static method()
  // Anything on the instance = method()
}

_db.js_

import Sequelize from 'sequelize';

// Create a connectiom and export it
export default connection = new Sequelize(/* connection */)

/*
//
// THAT'S IT!
//
// We now have `connection.models` containing our registered
// models, without _ever_ having to explicitly pass in 'connection'
// to them.
//
// This is achieved by storing a copy of the model class in its
// bare form inside a `WeakMap`, and then instantiating at the point
// of:
//
// a) Creating a connection
//
// and/or
//
// b) Using the @model decorator, which loops through the current
// `connections` Set and wires it up to each connections `.model` property,
// tying up relationships, etc.

Notes:

  • No need to pass in 'state' (i.e. a connection object), because at this stage, the @model decorator registers the class 'globally' inside the Sequelize library using a combination of WeakMap and Set and holds the class in memory until a connection is created.
  • Once a connection is created (i.e. const conn = new Sequelize(/* connection */), at that point, a "higher order" class is created that uses the map, injects the connection, and returns that back in conn.models. Any part of the app with a reference to conn can access all of the models, but the models themselves never need to worry about the connection.
  • This way, if more connections are created - even if the dialect or the underlying DB changes - the models will only reference the connection they're bound to. All other connections will retain their own 'injected' copies.
  • The class doesn't even have to extend Model, because at this point, it's just a vanilla/bare class with static and possible instance methods. Only when creating the connection does Model 'wrap' it and it get initialised and bound to a connection.

That's basically what I do with MobX and it works really well. You get to use decorators (yay!), but without the unnecessary boilerplate.

As a side effect, you can also do things like this:

const staging = new Sequelize(/* connection */);
const production = new Sequelize(/* connection */);

... and both connections have their own 'injected' models that are bound to that connection, meaning you can define them in one place, but be assured that they'll be scoped to the right sequelize at the time of defining the connection.

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Sep 2, 2016

@leebenson

... contains the connection, then feeding models into it with something like sequelize.addModel(ModelClass) feels like a solid design choice. Having to force it onto models means taking a model manager that could occur in one place, and adding boilerplate to every model. Not an issue for a single model... but at scale, it starts to add up.

Please note that no matter how you put it, currently a model needs to know about the connection. Eg find() will look for this.sequelize to make its query. And this connection needs to be passed into the model, through a static constructor. You can either do it after the declaration, or in you model index file, but there is no way around it. I actually also like the idea of registering the model with addModel() in the model index file if we had connection-independent models. But imo the initialization with attribute metadata should still happen in the model file.

I am 100% for connection-independent models, and then we could override the connection to use on every query, while the model registration is handled by the ModelManager (we already have model manager object in sequelize). But actually, when I think about it, I don't even know why we need one if we work with class references. When you associate two models, you pass the explicit class reference to the function and it gets saved under associations. So even when running an include with a string, we can get a reference to the included model from there. Besides that, I guess the only thing that we need it for is if you pass a string to belongsTo() etc, because we need to expand that (and in that case a WeakMap would of course not work 😉). But I think we try to move away from string references (for the good) and maybe we should deprecate this one day.


Node has great support, but I've always thought of circular dependencies as an anti-pattern.
Maybe this stems from my work with Go or other languages, but I'd generally agree with the points in the above link for any language, for the same reasons.

Yes, I know it is considered an anti pattern for the named reasons, and I agree with the points. But all of these don't apply in this case:

  • High coupling: Your classes are coupled, no need to sugar-coat it. They are two models that are in a relation. A depends on B and B depends on A. When you do it with circular dependencies, this is very explicit and constrained to only A and B. But when you do it with an injected models parameter, you are actually not only passing in A and B, you are passing in all of the models in your whole app. Even if you only use one model from this hash, there is no contract about this. So technically A is now not only coupled to B, it is coupled to all 100 models. This becomes apparent when you use TypeScript and try to type the models parameter. The type would look like {[name: string]: Model} - it's an attribute hash with all models that were defined, and nowhere is defined which models are actually in there and which the model actually needs. And even when you pass a model parameter, the circular dependency still exists. Dependency means a module cannot work without another module. An explicit import or passing the whole models hash in doesn't matter, A still depends on B and B on A. But by using explicit import statements - even if they are a cyclic - I am actually reducing coupling in my app (I already mentioned some examples in my last response how this benefits testing models or endpoints in isolation).

  • Static linking: JS compiles JIT, and TS compiles just fine

  • Crashing development tools: ESLint, TSLint and the TS language server all handle these imports fine. Moreover, the explicit import allows me to get autocompletion from my IDE. A typeless models parameter is of no meaning for a language server.

  • God objects: A model definitely is'nt one ;)

  • Circular entity references (especially in databases, but also in domain models) prevent the use of non-nullability constraints, which may eventually lead to data corruption or at least inconsistency.

    What they meant here was that you cannot create object A without creating B without creating A, so you have to create A first with a null reference to B, and then create B, and then replace the null reference with the B instance. This is how NodeJS circular require works, but the arguments of data corruption or inconsistency is neglectable here. As a developer following this pattern, I know I can only import the other models after the declaration, because they are not available before. I just have to follow the rule of late import.

  • Cognitive overhead: I accept this point, some developers might find it confusing to understand how this is possible. But to me, explicit imports read clearer than a magic models parameter. Suppose I don't know the codebase and just opened the model file - where does that come from? where is the method called? Where is the other model defined? An import leads me directly to the other module.


You are talking a lot about avoiding "state" in your models. But at least in master, a model never has any real state. It is completely static. You need to initialize it with the static constructor, and then the attributes and connection are defined and set in stone (except if you use removeAttribute() of course). I agree 100% that it would be nice to have connection-independent models, and then we could set a default connection on a model, which counts as state. But currently, there are only two states a model can have: Uninitialized and Initialized. The first one should be completely avoided, because the model cannot function in this state. It is like constructing a class without calling its constructor (an antipattern that is possible with Object.create() or reflection in other languages). So I like to call init() as soon as possible, directly after declaration, so the model is always guaranteed to be intialized. By deferring the init() call you will actually cause state, because the model is uninitialized until init() is called. You could import your model and it would be in an invalid state, not initialized, not associated. This cant happen with the import pattern: You import your model, and it is ready to use.

One can definitely feel the differences in our technology stacks ;) As a React guy you desire statelessness, immutability, reducers, pass-by-argument. As a TypeScript guy I desire type safety and editor autocompletion (+I can use property decorators).

@relationships({ belongsTo: ['User'], hasMany: ['SessionData']})

Not a fan of magic string constants. No explicit dependency expressed here, and you cannot get any autocompletion for the string. This is what put me off from PHP ORMs too, for example in doctrine you configure the relationships the same way, in a docblock:

    /**
     * @ManyToOne(targetEntity="Address")
     * @JoinColumn(name="address_id", referencedColumnName="id")
     */
    private $address;

The unique ability of JavaScript to work with classes as objects and pass them around makes it perfect for avoiding these kinds of magic strings, and I really like our current association syntax. Sequelize is really unique compared to other ORMs here because the language is so flexible. I don't see it as boilerplate if I get autocompletion and type safety in turn.

No need to pass in 'state' (i.e. a connection object), because at this stage, the @model decorator registers the class 'globally' inside the Sequelize library using a WeakMap and holds the class in memory until a connection is created.

That kind of goes against the semantic of decorator - the decorator should decorate a class with metadata (mutate it), not pass it to some higher instance. For this, I would prefer a sequelize.addModel() method to be called in the model index file. I also think you misunderstand WeakMap here, WeakMap would actually not keep it in memory because you reference it nowhere else, because you only use strings.

Once a connection is created (i.e. const conn = new Sequelize(/* connection */), at that point, a "higher order" class is created that uses the map, injects the connection, and returns that back in conn.models. Any part of the app with a reference to conn can access all of the models, but the models themselves never need to worry about the connection.

I think the whole discussion around connection-independent models should be taken to a new issue, but I don't see it coming soon, because I personally don't have a use case for multiple connections and internally it is an epic refactor. But going from the architecture, it would be nice if we could pass a connection at query-time and optionally set the default connection with init() or later by assigning Model.sequelize =.

This way, if more connections are created - even if the dialect or the underlying DB changes - the models will only reference the connection they're bound to. All other connections will retain their own 'injected' copies.

Thats exactly how it works with init() atm though aswell. A model is bound to a connection.

The class doesn't even have to extend Model, because at this point, it's just a vanilla/bare class with static and possible instance methods. Only when creating the connection does Model 'wrap' it and it get initialised and bound to a connection.

That is not true, the find() / create() methods need to come from somewhere, and inheritance is the best solution for this. Otherwise you would have to apply them as a mixin to every single one of the 100 models, and that would cost a ton of RAM.

@leebenson

This comment has been minimized.

leebenson commented Sep 2, 2016

Thanks @felixfbecker - I'm reading through your post, but I just wanted to clear something up...

Please note that no matter how you put it, currently a model needs to know about the connection. Eg find() will look for this.sequelize to make its query. And this connection needs to be passed into the model, through a static constructor. You can either do it after the declaration, or in you model index file, but there is no way around it. I actually also like the idea of registering the model with addModel() in the model index file if we had connection-independent models. But imo the initialization with attribute metadata should still happen in the model file.

That's not actually the case 😄

A model doesn't have to know about it connection at the time of definition.

You can store the class in a WeakMap and then when a connection is created via new Sequelize(), at that point inject it with a connection.

You don't have to do that ahead of time.

I say that from experience. I'm using decorators throughout my application that relies on per-request state (generated when a user visits my app), where things like local scoping become paramount.

None of my decorators require state explicitly. They wire it up implicitly at the time the connection is defined.

I'd highly encourage you to explore WeakMaps - they can solve this design problem very easily.

@leebenson

This comment has been minimized.

leebenson commented Sep 2, 2016

You are talking a lot about avoiding "state" in your models. But at least in master, a model never has any real state. It is completely static

When I say 'state', I mean it analogous to 'connection' - i.e. the sequelize object created by new Sequelize.

That is, in effect, state. It's a connection handler that defines an active connection pool, DB, etc. That's what I mean by 'state'.

Models themselves don't need state. They're just static definitions. My point is that you can inject connections into the models (via a higher-order class, like Model) without having to explicitly pass around sequelize to it. Again, check out WeakMap and Set - if you're not using both, you're not doing it right.

Your current design has two major flaws:

  • You have to create the sequelize object first. What if Models have already been defined?
  • For every connection stack (i.e. every sequelize object), you have to reinitialise (manually) all of the models that go along with that connection. If you're using decorators, you might as well store a cached copy of the class via a WeakMap so you can access them again, whenever you want, when a new sequelize object is created. Doing it the other way around is ass-backwards IMO.

tl;dr - if you want decorators, use WeakMap and Set.

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Sep 2, 2016

It is, internally. A Sequelize instance is way more than just a connection pool. You can set default model options, that get merged into the options inside init(). The usage of the sequelize property is far-flung, as we for example need to expand strings to models, and sequelize holds the other registered models. If you do it in your app, you are lucky that you don't use any of the features that expect sequelize to be present. If we want to keep supporting these, the sequelize attribute is required at definition time. In an ideal world it wouldn't, but this is how it is right now. And I do know WeakMap and Set ;) Saying if I don't use them I'm doing it wrong is a pretty bold statement, I argue against it, given that a sequelize instance contains a map of models already and is required at definition time anyway for the reasons stated above.

@leebenson

This comment has been minimized.

leebenson commented Sep 2, 2016

But @felixfbecker, you can still have all of that without explicitly passing around sequelize to every model. The complexity under the hood of Sequelize is irrelevant. The only thing you're changing is that .init() now happens automatically, behind the scenes, instead of at definition time.

All you need is a global 'cache' of models inside Sequelize.

When you connect them with a decorator like

@model({ id: DataTypes.STRING })
export class SomeTable {}

... then the @model decorator stores that cached class inside a WeakMap, and creates a Set pointing to the instance. The class is the key. The value is the configuration.

Then, whenever new Sequelize() is used (and a sequelize object created), the Set is iterated through, you create a higher-order class to extend the one in your WeakMap store, and run the Model.init with the new connection object.

Simple.

Nothing much needs to change under the hood. Just some caching and looping whenever a new model is defined and/or a new Sequelize is created.

@leebenson

This comment has been minimized.

leebenson commented Sep 2, 2016

The reason the above works, of course, is that all exports in Node are cached. So in your Sequelize library, you'd have a single reference like...

const modelConfig = new Weakmap;
const models = new Set;

... and whenever you use @model, it references the same models Set inside of the library.

Then whenever you do new Sequelize, you have a 'singleton' models cache that can be used as the 'base' class in which to inject and pair up with the current connection.

Your modelConfig WeakMap would contain the meta. This also solves bleeding out into variables like 'options' or 'attributes' - it's defined outside the class, by the decorator.

That's exactly how I do it, and it works beautifully.

@leebenson

This comment has been minimized.

leebenson commented Sep 2, 2016

Saying if I don't use them I'm doing it wrong is a pretty bold statement, I argue against it, given that a sequelize instance contains a map of models already and is required at definition time anyway for the reasons stated above.

I apologise if it came across rude, not my intention. I'm just trying to express, as clearly as possible, that if you want to use decorators, I really think WeakMap and Set will make the syntax beautifully simple.

If I had a bit more time right now, I'd hack together a demo to show you it working in practice.

@richthegeek

This comment has been minimized.

richthegeek commented Jul 10, 2017

I'd asked about this in Slack so here's a viewpoint developed prior to this looking at this thread:

https://gist.github.com/richthegeek/ffe78543c530f5bf20b3b7ae5d9d3350

Having read through the thread, there are a few things that aren't clear:

  1. What's the actual benefit of using decorators? Besides looking 'future' and requiring a transpiler for the forseeable future, it's not obvious what benefit they provide over just statically defining the fields and options.

  2. What's the point of making any of these changes if we still have a messy init function? Perhaps this confusion comes from it not actually being clear (without having looked at the Sequelize source) what db.define does currently. I assume that if it needs to modify how the columns are represented internally that the db.importClass function (or whatever is used to link a Model to a connection) would do that implicitly.

More generally it seems like the discussion is being kept quite forcefully away from an "ideal API" (which mine definitely isn't, but i'd say the primary features would be "minimal" and "almost native") and honing in (mostly pointlessly?) on either what would work without any changes to the Sequelize core or what would be 'CS-theoretical correct'.

If I have to use a transpiler to use this style of model definition then that'd be a massive failure.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jul 10, 2017

@richthegeek static will also require transpilation (i don't believe it's in the current LTS)
We definitely want an ideal design but there are limitations while Node.js does not support all of ES6 (your proposal is not possible at the moment as far as i know).

@richthegeek

This comment has been minimized.

richthegeek commented Jul 10, 2017

@mickhansen ah yes so it will :) in which case the schema method seems preferable?

@leebenson

This comment has been minimized.

leebenson commented Jul 10, 2017

The point of decorators is to annotate functions with new features, without changing the signature of the original function. When there's just one of them, it might seem like overkill; but you can imagine a scenario where you have @model to define a model, @relationship to define a link between a function and a parent, @uuid to represent a unique column type, etc.

They start to make more sense when you have a collection of well-designed decorators that can easily annotate a function without changing the inner function's implementation detail.

The way they're currently defined in Sequelize is, IMO, a bit redundant. You have to explicitly pass in the sequelize object, which contradicts some of the lightweight ease of 'decorating' a plain class/function. In this case, you might as well just define things statically.

I feel it'd make more sense if decorators followed a pattern like I demoed here.

Since writing that, I generally now do 95% of my DB work in SQLAlchemy for Python. It's not fair to compare it with Sequelize directly, because Python offers a bunch of first-class language features (decorators being one of them) that Node doesn't support natively (or at all). It can perform ORM gymnastics that simply wouldn't be possible in Javascript, due to the lack of operator overloading.

With that said, their general implementation is a joy to work with. You can define a global 'Base' and then further define models by simply extending Base. This is somewhat analogous to my suggested approach with decorators; you could export a global 'base' and then every call to @model would implicitly use that base, so that passing around an explicit sequelize object is redundant.

FWIW, if you're flexible on the back-end language, I'd highly encourage giving SQLAlchemy a look. The work that has gone into Sequelize is fabulous, but if you're not constrained to Node, there's no need to limit yourself to Javascript. Python's language features make an ORM an easy fit.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jul 10, 2017

@richthegeek I haven't followed the discussion too closely, defining models is something i do once and as long as it works i don't really care too much, the query API is a bigger surface area.

@leebenson SQLAlchemy is definitely a beaut to work with.

@richthegeek

This comment has been minimized.

richthegeek commented Jul 10, 2017

@leebenson I'm not sure that I understand this: "annotate a function without changing the inner function's implementation detail"

The definition of the model is the core of the implementation. What do you actually mean by this?

@leebenson

This comment has been minimized.

leebenson commented Jul 10, 2017

@richthegeek - a decorator takes the original function, and generally returns a modified version of that same function.

So the point of @model would be that it provides the passed function access to the table that it's modelling, rather than leaving it up to the function (or class) to define it.

Basically -- the responsibility is then handed off to Sequelize, rather than it being something that happens in userland by adding statics to classes, etc.

Like I said, it's a subtle difference when there's only one decorator in play, but it makes sense when there's a few of them that hide away the implementation-- you can avoid adding 'noise' to your classes by requiring that, say, init() is necessary on each of your custom model classes and instead define that stuff at the decorator level.

@snewell92

This comment has been minimized.

snewell92 commented Jul 10, 2017

It's worth noting the class+decorator syntax has been implemented fully with Typescript. And usage is very straightfoward without a complicated init.

Is that what you're looking for? It's not quite as fully featured as the ORM in python you mentioned, but it's something! :D

(although it's also worth mentioning this is just using a transpiler, which has been mentioned before.)

@epicbagel

This comment has been minimized.

epicbagel commented Jul 10, 2017

@leebenson one option is to spin this off into a separate library (e.g. sequelize-model-builder) that could hide some of the messiness with "init" rather than working to get this into Sequelize while annotations still require transpilation.

It's something I've considered working on. We have a large number of model definitions and the approach you posted above is along the lines of the direction we'll like to take.

@dcworldwide

This comment has been minimized.

dcworldwide commented Jul 10, 2017

For typescript users I think typeorm completely nailed their orm api. https://github.com/typeorm/typeorm. Well worth reviewing for inspiration.

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Jul 10, 2017

What exactly is the "messiness" with init? It's just a static constructor. It needs to normalise options, and initialises the model. This is not possible with static constructors, and decorators are simply a way to invoke that logic.


Both typeorm and sequelize-typescript use terrible patterns for associations that will plain out break with native ES6 modules in Node:

@HasMany(() => User)

this is so incredibly hacky, a cyclic dependency that only works because of TS compiler implementation details when transpiling to CommonJS.

Why does a "TypeScript library" for Sequelize have so many lines of code?!
https://github.com/felixfbecker/sequelize-decorators is 39 lines and requires no init call either.

@snewell92

This comment has been minimized.

snewell92 commented Jul 10, 2017

re: "terrible patterns" Ough! Ouch. 🗡 💀

re: messiness
In regards to the messiness... yeah I stand with felix on this one. It's just not messy enough really messy at all to warrant wrapping the initialization process nor factoring that out. In my context, I prefer the single class with minimal decorator approach because that communicates more to my team.

However, if I wasn't constrained by communicating to an older team, I would rather just use sequelize plainly ('idiomatically' if you will). If I wanted to cut down on lines of code, the decorator syntax OR static init would be leaner than what I have now.

None of what @felixfbecker or @leebenson have suggested above is messy, it's an improvement over what existed.

If you disagree, provide code samples to try to demonstrate some friction in the way you define your models and the way you wish you could define them, and then discussion can proceed, but I'm not seeing anything messy (even as someone who prefers the class approach).

Implementation aside

Also, if the friction is just a repeated init function for all your models, and you think it's violating SRP... you can factor that out in its own function, or perhaps have all your models inherit from the same prototype, or perhaps do a more oopy 'base class' dealio.

The problem you'd run into is I'm willing to bet not all your init functions are created equal. There's some good reasons the sequelize team has it still.

As it stands, Sequelize lets you choose how to do it. We are using JavaScript after all , anything is possible !

You could even create the model builder yourself as a specialized model factory if you really wanted to. If you want it, just do it! Then share it here and the sequelize team will use what they think is valuable, and keep in userland what they believe to be best left in userland.

@leebenson

This comment has been minimized.

leebenson commented Jul 10, 2017

We are using JavaScript after all, anything is possible!

Except operator overloads, first-class decorator support, list comprehensions, transactions that don't get lost inside of Promise chains... 😛

@keyiis

This comment has been minimized.

keyiis commented Aug 21, 2017

sequelize come on! ! ! i won't change to another orm(typeorm),i hope decorator become standard library in sequelize code and doc!

@RobinBuschmann

This comment has been minimized.

RobinBuschmann commented Aug 26, 2017

@HasMany(() => User)
this is so incredibly hacky, a cyclic dependency that only works because of TS compiler implementation details when transpiling to CommonJS.

@felixfbecker This is not true. ES6 modules are indeed able to handle cyclic dependencies. So this is not hacky at all and has nothing to do with 'implementation details' of the tsc.

There are some browser(chrome canary, safari technology preview) out there, which implements ES6 modules natively. If you run the following code snippets (which includes a cyclic dependency) in one of these browsers, you will see that it works very well.

 // a.js
import {b} from './b.js';

export function a() {
    return b();
}

export function a1() {
    return 'a1';
}
// b.js
import {a1} from './a.js';

export function b() {
    return 'b' + a1();
}
// index.js
import {a} from './a.js';

console.log(a());
<script type="module" src="index.js"></script>
@zhanwenchen

This comment has been minimized.

Contributor

zhanwenchen commented Sep 15, 2017

After all night of tweaking, I finally have a working example, based on @leebenson and @felixfbecker's posts on this thread. @snewell92 take a look at this.

@felixfbecker one needs to explicitly return super.init({...}, {sequelize}); in static init(...). The child init function should also take a sequelize instance.

To use, put all 4 files in an empty directory and do node index.js.

@mickhansen In my example, using Node 6.11.3 LTS, I did not need a transpiler for static.

index.js
// models/index.js
/**
  index.js is an import utility that grabs all models in the same folder,
  and instantiate a Sequelize object once for all models (instead of for each model).
  This is done by passing the single Sequelize object to each
  model as a reference, which each model then piggy-backs (sequelize.define())
  for creating a single db class model.
*/

"use strict"; // typical JS thing to enforce strict syntax

const fs = require("fs"); // file system for grabbing files
const path = require("path"); // better than '\/..\/' for portability
const Sequelize = require("sequelize"); // Sequelize is a constructor
const env = process.env.NODE_ENV || "development"; // use process environment
const config = require(path.join(__dirname, '..', 'config.js'))[env] // Use the .config.json file in the parent folder
const sequelize = new Sequelize(config.database, config.username, config.password, {
  dialect: config.dialect,
});

// Load each model file
const models = Object.assign({}, ...fs.readdirSync(__dirname)
  .filter(file =>
    (file.indexOf(".") !== 0) && (file !== "index.js")
  )
  .map(function (file) {
    const model = require(path.join(__dirname, file));
    // console.log(model.init(sequelize).tableName)
    return {
      [model.name]: model.init(sequelize),
    };
  })
);

// Load model associations
for (const model of Object.keys(models)) {
  typeof models[model].associate === 'function' && models[model].associate(models);
}

module.exports = models;
Post.js
// models/Post.js
/**
  Post.js
  Class model for Post
*/

'use strict';

const Sequelize = require('sequelize');

module.exports =
  class Post extends Sequelize.Model {
    static init(sequelize) {
      return super.init({
        title: {
          type: Sequelize.STRING,
          allowNull: false,
        },
        body: {
          type: Sequelize.TEXT,
          allowNull: false,
        },
        assets: {
          type: Sequelize.JSON,
          allowNull: true
        },
      }, { sequelize })
    };

    static associate(models) {
      // Using additional options like CASCADE etc for demonstration
      // Can also simply do Task.belongsTo(models.Post);
      this.hasMany(models.Comment, {
        onDelete: "CASCADE",
        foreignKey: {
          allowNull: false
        }
      });

      // Using additional options like CASCADE etc for demonstration
      // Can also simply do Task.belongsTo(models.Post);
      this.belongsTo(models.User, {
        onDelete: "CASCADE",
        foreignKey: {
          allowNull: false
        }
      });
    }
  }
User.js
// models/User.js
/**
  User.js
  Class model for User
*/

'use strict';

const Sequelize = require('sequelize');

module.exports =
  class User extends Sequelize.Model {
    static init(sequelize) {
      return super.init({
        firstName: {
          type: Sequelize.STRING,
          allowNull: false,
        },
        lastName: {
          type: Sequelize.STRING,
          allowNull: false,
        },
        username: {
          type: Sequelize.STRING,
          allowNull: false,
        },
        password_hash: {
          type: Sequelize.STRING,
          allowNull: false,
        },
        salt: {
          type: Sequelize.STRING,
          allowNull: false,
        },
        email: {
          type: Sequelize.STRING,
          allowNull: false,
          validate: {
            isEmail: true
          }
        },
        isActive: {
          type: Sequelize.BOOLEAN
        }
      }, { sequelize })
    };

    static associate(models) {
      // Using additional options like CASCADE etc for demonstration
      // Can also simply do Task.belongsTo(models.User);
      this.hasMany(models.Post, {
        onDelete: "CASCADE",
        foreignKey: {
          allowNull: false
        }
      });

      // Using additional options like CASCADE etc for demonstration
      // Can also simply do Task.belongsTo(models.User);
      this.hasMany(models.Comment, {
        onDelete: "CASCADE",
        foreignKey: {
          allowNull: false
        }
      });
    }
  }
Comment.js
// models/Comment.js
/**
  Comment.js
  Class model for Comment
*/

'use strict';

const Sequelize = require('sequelize');

module.exports =
  class Comment extends Sequelize.Model {
    static init(sequelize) {
      return super.init({
        title: {
          type: Sequelize.STRING,
          allowNull: false,
        },
        body: {
          type: Sequelize.TEXT,
          allowNull: false,
        }
      }, { sequelize })
    };

    static associate(models) {
      // Using additional options like CASCADE etc for demonstration
      // Can also simply do Task.belongsTo(models.Comment);
      this.belongsTo(models.User, {
        onDelete: "CASCADE",
        foreignKey: {
          allowNull: false
        }
      });

      // Using additional options like CASCADE etc for demonstration
      // Can also simply do Task.belongsTo(models.Comment);
      this.belongsTo(models.Post, {
        onDelete: "CASCADE",
        foreignKey: {
          allowNull: false
        }
      });
    }
  }
@snewell92

This comment has been minimized.

snewell92 commented Sep 15, 2017

Dang, neat!

Related tidbit

This is not strictly related, but I do something similar for initializing services in feathers by using fs to read a directory structure.

One thing I commented on for a future item, is that if any of my services depend on one another, the way I'm initializing my services won't work.

However, the way sequelize splits out associating models together after the models are all initialized could apply to that domain too. And thus, as long as my services aren't using each other in the init phase, I could compose them or use them in the associate phase. Exactly how your POC works.

@sylv3r

This comment has been minimized.

Contributor

sylv3r commented Nov 6, 2017

Great post and thanks @zhanwenchen for this very detailed example which helped me a lot !

However I'm still having issues figuring out how to define hooks, getters and setters in this case. I tried static methods, instance methods, direct definition like MyModel.beforeCreate = ..., etc. I'm unable to get it triggered whatever I try.

Would you mind to give an example of the right way to do it please ?

@UnbrandedTech

This comment has been minimized.

UnbrandedTech commented Jan 17, 2018

@sylv3r SUPER late to the game but I think you can add it to the init options...

class User extends Model {
    static init(sequelize){
        return super.init({
           //definition
        }, {
            sequelize,
            hooks: {
                beforeCreate: User.beforeCreate,
                afterCreate: User.afterCreate,
                beforeUpdate: User.beforeUpdate
            }
        })
    }
}

Haven't tested this, still researching if this is an ok pattern to follow. (The whole class extends thing)

@skrubinator88

This comment has been minimized.

skrubinator88 commented Feb 15, 2018

Wow this helped me a lot, thanks a ton!

@jannikkeye

This comment has been minimized.

jannikkeye commented Mar 17, 2018

Is there a reason why this approach is not documented anywhere in the documentation? Should I be using this or the normal .define way?

@UnbrandedTech

This comment has been minimized.

UnbrandedTech commented May 2, 2018

Just to confirm, my previous model does indeed work and is in production.

@tsirolnik

This comment has been minimized.

tsirolnik commented Oct 15, 2018

Anyone had success with instance methods? Can't make it work for some reason. https://stackoverflow.com/questions/52813078/sequelize-es6-model-methods-no-existing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment