website icon indicating copy to clipboard operation
website copied to clipboard

Standardizing/documenting a way to create plugins for Sequelize

Open cusspvz opened this issue 10 years ago • 52 comments

Since sequelize is relaying on hooks to comunicate with plugins, shouldn't be better to include a method so we can pass a sequelize instance to plugins?

I would propose a similar socket.io and express.js approach...

API Proposal

Specs for Sequelize.prototype.plugin

1 - .plugin method should communicate with plugins by passing to the plugin's function: sequelize instance as 1st argument, and options object as 2nd argument. 1.1 - If first argument is a function, it should only call it with sequelize instance. 1.2 - For the laziest and KISS fans, if a string is provided instead as .plugin's first argument, .plugin method should use require internally. 1.2.1 - We should be able to require relational paths 1.2.1.1 - If an wild card exists, it should search for .js files and require them all recursively. 1.2.2 - We should be able to require node modules 1.3 - If first argument is an array, it should call it self recursively with each value 2 - Options handling 2.1 - Second argument of .plugin should be optional and be passed to plugin always as an object 2.2 - If plugin's function has the key defaultOptions defined as an object, .plugin should extend options, if provided, from it. 3 - .plugin should throw an error if something went wrong. 4 - .plugin should return sequelize instance always for chain proposes.

Specs for Sequelize.plugin

1 - Should proxy into Sequelize.prototype.plugin by passing args context within afterInit hook

Core Implementation Example
Sequelize.plugin = function ( plugin, options ) {
  return this.addHook( 'onInit', function ( sequelize ) {
    sequelize.plugin( plugin, options );
  });
};

Plugging Examples

/*
 *  Plugging into Sequelize Class ( they will be applied on all future Sequelize instances )
 */
Sequelize.plugin( 'sequelize-cache-redis' );

/*
 *  Plugging into Sequelize instance (won't be applied on other Sequelize instances )
 */
var sequelize = new Sequelize( /* ... */ );

sequelize.plugin( 'sequelize-cache-redis' );

/*
 *  Plugging multiple plugins into a Sequelize Class by chaining
 */
Sequelize
  .plugin( 'sequelize-cache-redis' )
  .plugin( 'sequelize-tree' );

/*
 *  Plugging multiple plugins into a Sequelize Class by an array with names
 */
Sequelize
  .plugin([ 'sequelize-cache-redis', 'sequelize-tree' ]);

/*
 *  Plugging into a Sequelize Class from a folder of plugins
 */
Sequelize.plugin( './models/plugins/*' );

/*
 *  Plugging into a Sequelize Class providing options
 */
Sequelize.plugin( 'plugin-name', { foo: 'bars' } );

/*
 *  Plugging the same plugin into different Sequelize instances with different options
 */
var localRedisClient, sharedRedisClient;

var mysql = new Sequelize( /* mysql config */ );
var sqlite = new Sequelize( /* local sqlite config */ );

mysql.plugin( 'sequelize-cache-redis', { redis: sharedRedisClient });
sqlite.plugin( 'sequelize-cache-redis', { redis: localRedisClient });

Plugin Examples

/*
 *  Plugin skeleton example
 */
module.exports = function ( sequelize, options ) {
  // ...
};

// Default options definition
module.exports.defaultOptions = {
  somePluginOption: false,
};
/*
 *  Cache Plugin example
 */
module.exports = function ( sequelize, options ) {
  if( ! options.redis ) return;

  sequelize.addHook( 'afterDefine', function ( Model ) {
    if( ! Model.options.cache ) return;

    Model.addHook( 'beforeFind', function ( queryOptions ) {
      // ...
    });

    Model.addHook( 'afterFind', function ( queryOptions ) {
      // ...
    });
  });
};

// Default options definition
module.exports.defaultOptions = {
  redis: false,
};

Opinions and suggestions? @overlookmotel @mickhansen @janmeier

cusspvz avatar Oct 31 '14 00:10 cusspvz

I just tend to think of middleware whenever i see .use().

mickhansen avatar Oct 31 '14 08:10 mickhansen

@mickhansen if you like the idea, but not the name, we can provide others :)

  • .plug()
  • .plugin()
  • .ext()
  • ...

cusspvz avatar Oct 31 '14 19:10 cusspvz

I was thinking, it would be nice to plug an entire folder or regexp path like ./plugins/*.sequelize.js. It should be easier for private plugins. :p

cusspvz avatar Oct 31 '14 19:10 cusspvz

Sorry for silence... I've been tied up with other things.

I agree that it'd be a good idea to agree a standard way for plugins to integrate into Sequelize. I also opened a discussion about this here: https://github.com/sequelize/sequelize/issues/2462. Sorry, @cusspvz, I should have pinged you on it.

I can see the benefit of your approach @cusspvz of attaching plugins into an instance of sequelize rather than into the constructor Sequelize, as it allows different instances of sequelize to have different plugins attached to them.

However, it prevents you doing some things which could be useful for plugin authors.

Two examples from plugins I've written:

  1. In sequelize-extra, I've augmented Sequelize with three extra data types DATETIME, TIME and DATEONLY
  2. In sequelize-virtual-fields, I've monkey-patched the get() method of Sequelize.Instance

Both of those require being able to change Sequelize rather than an instance sequelize - which your approach wouldn't allow. And I think both are the kind of things that it would be quite common for a plugin author to want to do.

However, I do agree that you should be able to use a plugin on one instance of sequelize and not use it on another instance. Therefore, for sequelize-virtual-fields, the plugin is installed globally, but has to be activated on an instance of sequelize by calling sequelize.initVirtualFields(). If you haven't called that method on a particular instance of sequelize, it behaves exactly as it would without the plugin being installed.

Maybe an approach along those lines is the best of both worlds?

overlookmotel avatar Nov 13 '14 01:11 overlookmotel

By the way, I like the flexible use method you've suggested above @cusspvz (or call it plugin or whatever). But I'd suggest it be attached to Sequelize rather than sequelize.

e.g.:

var Sequelize = require('sequelize');
Sequelize.use(require('some-sequelize-plugin'));

The form of the plugin code would then be:

module.exports = function (Sequelize) {
    // ... augment Sequelize to do the business ...
}

NB to attach methods etc to sequelize, the plugin can create afterInit hooks which run after new Sequelize(...) is called.

overlookmotel avatar Nov 13 '14 01:11 overlookmotel

I can see the benefit of your approach @cusspvz of attaching plugins into an instance of sequelize rather than into the constructor Sequelize, as it allows different instances of sequelize to have different plugins attached to them.

For me this is the most important thing! Plugins should couple within instances and not Classes / Prototypes

In sequelize-extra, I've augmented Sequelize with three extra data types DATETIME, TIME and DATEONLY

You could create an object prototyping Sequelize prototype and exchange things to serve only on a specific instance without affecting other instances behavior / prototype.

Example:

/* Plugin export */
module.exports = function ( sequelize /* instance */, options ) {
  var proto = sequelize.prototype = Object.create( sequelize.prototype );

  // and do whatever you want, and if needed, repeat the process
  // proto.DATETIME = Object.create( proto.DATETIME );
  // proto.DATETIME.var = 'something';

}

module.exports.defaultOptions = {
  your: 'defaults'
};

cusspvz avatar Nov 13 '14 15:11 cusspvz

I agree that it'd be a good idea to agree a standard way for plugins to integrate into Sequelize. I also opened a discussion about this here: sequelize/sequelize#2462. Sorry, @cusspvz, I should have pinged you on it.

I should also searched better before posting this... :p

cusspvz avatar Nov 13 '14 15:11 cusspvz

By the way, i would vote for .plug method name :) ping @mickhansen @janmeier @overlookmotel

cusspvz avatar Nov 13 '14 15:11 cusspvz

bump!

I need to create some plugins for sequelize so it would be awesome if we decide the best way to plug them. :)

cusspvz avatar Nov 25 '14 14:11 cusspvz

A plugin method on the sequelize constructor and making it convention for plugins to attach their behaviour on afterInit / when an init function is called sounds good to me

janmeier avatar Nov 27 '14 14:11 janmeier

@janmeier do you agree with the current method proposal?

cusspvz avatar Nov 27 '14 15:11 cusspvz

If I read him right, I think @janmeier is saying that the plugin method in his opinion should be on the Sequelize constructor, not on an instance of sequelize. Same as my opinion! :)

Again, I do see what you're driving at with your approach, but I think it's easier to attach plugins toSequelize itself. Then you can package up an augmented version of Sequelize in a file which can be require()-d in anywhere in your code - no need to keep passing around an instance of sequelize.

I think the important thing is to write up a set of "rules" that plugins should follow so that they don't cause changes to the native behaviour of Sequelize unless the user specifically requests it.

That way, you can have multiple sequelize instances which all use different plugins - which is what you are after.

Next question: how to handle namespaces? Once a lot of plugins get written, they'll start clashing with each other at some point. 3 things I can see that need to be namespaced:

  1. Additional methods added to Sequelize's native objects (Model, Instance etc)
  2. Options stored in sequelize
  3. Additional options passed to e.g. findAll() which trigger the plugin to do something

Any thoughts?

overlookmotel avatar Nov 27 '14 18:11 overlookmotel

I'm torn between whether or not plugins should be on Sequelize or the sequelize instance, i'd like to have different sequelize instances seperated from each other as much as possible (especially if we would like to pull dialects out as modules at one point)

mickhansen avatar Nov 27 '14 20:11 mickhansen

Yep, I agree with @overlookmotel

janmeier avatar Nov 28 '14 07:11 janmeier

I agree with @mickhansen , sequelize instances could be configured with different dialects on the same process. Even if they are with same dialects, i don't think that modifying the prototype directly is a good behavior since plugins could mess up things by changing the same vars.

cusspvz avatar Nov 28 '14 12:11 cusspvz

And if .plugin method behaves differently if called on Sequelize or on a sequelize instance?


// Like
Sequelize.plugin = function ( plugin, options ) {
  this.addHook( 'onInit', function ( sequelize ) {
    sequelize.plugin( plugin, options );
  });

  return this;
};

Sequelize.prototype.plugin = function ( plugin, option ) {
  // API proposal
};

cusspvz avatar Nov 28 '14 12:11 cusspvz

@overlookmotel imagine a sequelize-cache, how would you activate it with different options on different sequelize instances?

cusspvz avatar Nov 28 '14 12:11 cusspvz

@cusspvz Here's how you'd do a cache. Two ways:

Method 1

// this is the code for the plugin
module.exports = function(Sequelize) {
    Sequelize.prototype.initCache = function(cacheOptions) {
        // code to implement cache here
    };
};

To activate caching, the user does:

var sequelize = new Sequelize(database, username, password);
sequelize.initCache( ... );

Method 2

// this is the code for the plugin
module.exports = function(Sequelize) {
    addHook('afterInit', function(sequelize) {
        var cacheOptions = Sequelize.options.cache;
        if (!cacheOptions) return;

        // code to implement cache here
    });
};

To activate the cache, the user does:

var sequelize = new Sequelize(database, username, password, {cache: ... });

Both methods work, it's a matter of style which you prefer. Personally I like method 2.

The important thing to note is that using either method, you can also create another instance of sequelize in the same process:

var sequelize2 = new Sequelize(database, username, password);

...and it doesn't have caching enabled.

overlookmotel avatar Nov 29 '14 00:11 overlookmotel

The advantage of this approach is that you can put this in a file:

// mySequelize.js
module.exports = require('sequelize').plug(require('sequelize-plugin'));

and then use Sequelize = require('mySequelize') anywhere in your code where you previously did Sequelize = require('sequelize'). i.e. you've just made a drop-in replacement for Sequelize that you can use anywhere you want.

overlookmotel avatar Nov 29 '14 00:11 overlookmotel

Yep at what @overlookmotel said, especially regarding the last part about "plugging" the plugin in one place, in the file where you require sequelize

janmeier avatar Nov 29 '14 18:11 janmeier

The code i've posted 3 days ago, covers that case:

// Like
Sequelize.plugin = function ( plugin, options ) {
  this.addHook( 'onInit', function ( sequelize ) {
    sequelize.plugin( plugin, options );
  });
  return this;
};

With this case, you could also provide options to all newly created instances of sequelize. And also being able to use [sequelize_instance].plugin( name, options ) on a scope outside of Sequelize class.

cusspvz avatar Dec 01 '14 15:12 cusspvz

And it can be more cleaner if you check the API proposal i gave:

// mysequelize.js

module.exports = require( 'sequelize' )
  .plugin( 'sequelize-redis-cache' )
  .plugin( 'another-awesome-module', { foo: 'bar' } )


var Sequelize = require( './mysequelize' );

// ...
var cloud = new Sequelize( /* ... */ ),
  master = new Sequelize( /* ... */ ),
  sqlite = new Sequelize( /* ... */ );

// And plug some extra plugins on one of them
cloud.plugin( './cloud-plugins/*' );

// Or even the same with different options
sqlite.plugin( 'sequelize-counter-cache', { type: 'fullcount' });
master.plugin( 'sequelize-counter-cache', { type: 'atomic' });

cusspvz avatar Dec 01 '14 15:12 cusspvz

@cusspvz Well, it can certainly work either way. I prefer one way, you seem to prefer another and I don't think our arguing about it is really getting us any further. Neither of us seems willing to change our minds!

I've outlined as best I can why I think it's better to attach plugins to the Sequelize constructor.

I do however really like the rest of the API you've proposed with allowing loading plugins by providing a name string, especially allowing * wildcards.

Although I favour one way, it's clear that there's arguments on both sides. At this point in my opinion it's more important that a decision is made one way or another, than what decision is made!

I'd suggest that we stop squabbling now and leave it to @mickhansen and @janmeier to scratch their heads on this one and decide...

overlookmotel avatar Dec 01 '14 17:12 overlookmotel

Also, just to say, I don't have time to implement this function myself. So perhaps there's an argument that the opinion of whoever is actually going to implement should have more weight...

I would very much welcome a framework being set out, so I can standardize the various Sequelize plugins I've written.

overlookmotel avatar Dec 01 '14 17:12 overlookmotel

I've outlined as best I can why I think it's better to attach plugins to the Sequelize constructor.

@overlookmotel on my project, I've gonna use another sequelize instance for creating a local and temporary database on sqlite.

Therefore i need it to be coupled into sequelize instances so they just mess up with each other, i see javascript world as divided scopes for different proposes, i would vote them to be couple on instances even if i wouldn't need it.

I'd suggest that we stop squabbling now and leave it to @mickhansen and @janmeier to scratch their heads on this one and decide...

@overlookmotel I really think that @mickhansen, @sdepold and @janmeier should decide it.

Also, just to say, I don't have time to implement this function myself. So perhaps there's an argument that the opinion of whoever is actually going to implement should have more weight...

No matter what choice they take I could implement, since we need it on our [emvici] project.

cusspvz avatar Dec 01 '14 18:12 cusspvz

The latest proposal @cusspvz is appealing to me, but i'll have to give it some more thorough thought.

mickhansen avatar Dec 03 '14 19:12 mickhansen

I've updated specs and also added some better examples on my first comment.

I'm thinking about @overlookmotel needs so we can get them covered also, specially the cases that need to make some Sequelize Class changes.

API Proposal

Specs

1 - .plugin method should communicate with plugins by passing to the plugin's function: sequelize instance as 1st argument, and options object as 2nd argument. 1.1 - If first argument is a function, it should only call it with sequelize instance. 1.2 - For the laziest and KISS fans, if a string is provided instead as .plugin's first argument, .plugin method should use require internally. 1.2.1 - We should be able to require relational paths 1.2.1.1 - If an wild card exists, it should search for .js files and require them all recursively. 1.2.2 - We should be able to require node modules 1.3 - If first argument is an array, it should call it self recursively with each value 2 - Options handling 2.1 - Second argument of .plugin should be optional and be passed to plugin always as an object 2.2 - If plugin's function has the key defaultOptions defined as an object, .plugin should extend options, if provided, from it. 3 - .plugin should throw an error if something went wrong. 4 - .plugin should return sequelize instance allways for chain proposes.

cusspvz avatar Dec 05 '14 15:12 cusspvz

Any further thoughts on this anyone?

overlookmotel avatar Dec 28 '14 19:12 overlookmotel

I still need to go through all this again and have a discussion with @janmeier

mickhansen avatar Dec 29 '14 08:12 mickhansen

Close to finishing up 2.0 and would like to focus on this after that.

Do we have any examples of other javascript systems that have plugin/package support that we could take a look at aswell?

mickhansen avatar Jan 07 '15 12:01 mickhansen