Official plugin listing?

[ackerdev]

Is there any desire to provide an official plugin listing? I've created two plugins which others may benefit from: objection-timestamp and objection-softdelete.

[devinivy]

I would be interested in 1. defining best practices around creating model "plugins." and 2. a place to put software we create around objection, such as this stuff that @ackerdev has created and some things we have created (i.e. schwifty).

[newhouse]

+1 for best practices, followed by listing.

[wswoodruff]

+1 for listing, preceded by best practices.

[fl0w]

Plugins as a sub class is somewhat an anti pattern.

Consider having 10 separate plugins, that would been 10 deep prototype inheritance, which would be allot of trips to get the first.

Before there's a best practices and a listing, there should be an official plugin design to hook into objection somehow. At current stage any best practice is actually an anti pattern and a false sense of "I'm following convention" feeling.

[koskimas]

I should do this as soon as possible. I've been meaning to, but never got around to it.

@fl0w You are absolutely right about inheritance not being the best solution. I think the best practice is actually monkey patching, as crazy as it sounds.

Inheritance creates long prototype chains and deep nested constructor calls. v8 can deal with deep prototype chains, but nested constructor calls cost.

Adding a plugin mechanism, for example a Plugin base class, that would be flexible enough would be really intrusive and still limiting on what you could do with plugins.

So for example if your plugin adds a $beforeUpdate hook you would do this:

const oldHook = objection.Model.prototype.$beforeUpdate;
objection.Model.prototype.$beforeUpdate = function () {
  const res = oldHook.apply(this, arguments);

  // Prepare for other plugins that do something async.
  return Promise.resolve(res).then(res => {
    // Do your thing.
    return res;
  });
};

thoughts?

[koskimas]

Maybe we could add a objection.plugin(yourPluginFunc, options) methods just for clarity. The plugin method would actually do nothing but:

objection.plugin = (plugin, options) => {
  plugin(objection, options);
  return objection;
};

but it would provide a clear way to register plugins. Instead of having this in the beginning of your app.js:

const objection = require('objection');

aPlugin(objection.Model);
someOtherPlugin.register(objection);
objection = someThirdPlugin.apply(objection);

You would have this.

const objection = require('objection');

objection
  .plugin(aPlugin)
  .plugin(someOtherPlugin)
  .plugin(someThirdPlugin)

[fl0w]

Would it be over engineering to have BaseModel > Model > PluggableModel.
PluggableModel would be the "clear interface" of what's publicly pluggable, even possibly add on beforePlugin/afterPlugin-hooks. Pluggable could have a flat list of things to do on base hooks such as onBuild.

class PluggableModel extends Model {
  static plugins = {
    $beforeUpdate: [ ... ] 
  }

  $beforeUpdate () {
    // loop apply through this.constructor.plugins.$beforeUpdate
  }
}

This would allow user to define a specific plugin set for a model in cases where performance has to be considered.

Edit Maybe not even subclass it, just stick it in Model?

[koskimas]

Do we really need to limit what's publicly pluggable? We have no idea what kind of plugins people want to write. They could add custom methods to Model or QueryBuilder, replace validation with another, add support for rxjs observables, add support for table aliases etc. Overriding hooks is just one of the things. What would beforePlugin do for a plugin that adds a custom QueryBuilder method?

[fl0w]

Not sure, but isn't it quite plausible to break plugins which do stuff to internals which are subject to change (i.e. not public API)? Maybe that's not of concern though.

As far as beforePlugin, I was thinking maybe plugin probe:ing? To be fair, that could be done within the plugin itself.

I was mainly just throwing a different solution [to yours] to have something to compare against.

[fl0w]

Yours is definitely more powerful as it'd hook into objection, not QueryBuilder alone.

[koskimas]

It should be clearly stated in the best practices that plugging into methods or classes that are not documented (are not public) is not a good idea.

[fl0w]

That's reasonable and fair!

[wswoodruff]

Can we think of any situations where a plugin would need dependencies (plugins that rely on others)? HapiJS handles this with Topo. We should think of any special options that might be needed when registering plugins. Also, could we slap an 'enhancement' label on this issue?

[koskimas]

What if the plugins were modules like this:

module.exports = {
  name: 'my-plugin',
  dependencies: ['a-plugin', 'some-other-plugin'],
  register(objection, options) {
    // Do your stuff.
  }
};

And they were registered like this:

objection.plugins([
  require('my-plugin'),
  require('a-plugin'),
  require('some-other-plugin')
]);

Objection would call the register functions in the correct order based on dependencies. The following would throw because one of the deps is missing.

objection.plugins([
  require('a-plugin'),
  require('my-plugin')
]);

Options could be passed like this:

objection.plugins([
  {plugin: require('my-plugin'), options {foo: 'bar'}},
  require('a-plugin'),
  require('some-other-plugin')
]);

or would this be cleaner:

objection.plugins([
  [require('my-plugin'), {foo: 'bar'}],
  require('a-plugin'),
  require('some-other-plugin')
]);

?

[koskimas]

Or maybe objection should not even handle the options. If the plugin takes options it could simply be defined like this

module.exports = (options) => ({
  name: 'my-plugin',
  dependencies: ['a-plugin', 'some-other-plugin'],
  register(objection) {
    // Do your stuff.
  }
});

and used like this:

objection.plugins([
  require('my-plugin')({foo: 'bar'}),
  require('a-plugin'),
  require('some-other-plugin')
]);

[devinivy]

I'm a little concerned that we've begun over-engineering! I'm not super into monkey-patching objection's shared Model (and I don't think it addresses the "deep nested constructor" issue). We write componentized applications with objection, so each app component needs to have faith that Objection.Model is untouched, since it doesn't know what the other components are up to or whether they'll even be present in the application. Simple example– imagine one component giving its models a timestamp using a "plugin," but the other components not wanting to use a timestamp.

What we're talking about are mixins. So for now I don't think we would need anything more sophisticated than plain JS (order-reliant) mixins. I can appreciate that long prototype chains would have a cost, but at least that cost would be transparent to the user. Objection making extensive use of classes while spurning use of extends definitely causes a bit of dissonance in my fragile lil brain :P.

Also worth keeping in mind that other libraries, such as Polymer, once had complex plugin/mixin "behaviors" like we're talking about above, and eventually decided to drop them in favor of plain JS class mixins: https://www.polymer-project.org/2.0/docs/upgrade#mixins

Example,

const MyMixin = (BaseModel) => class extends BaseModel {
    $beforeUpdate() {
        // Prepare for other plugins that do something async.
        return Promise.resolve(super.$beforeUpdate()).then(res => {
            // Do your thing.
            return res;
        });
    }
};

class User extends MyMixin(Objection.Model) {}

[wswoodruff]

I like that! The user should have control over exactly what their base Objection class is going to look like at any point. I think being able to choose between different mixin classes and compose them as desired is better than running each plugin on the BaseModel class and forcing each feature for the whole project. There definitely will be situations where functionality from plugins will be wanted in certain places, and not wanted in other places. One of Objection's most shining attributes is the transparency of what features your Model is going to have, we don't wanna take that away

[koskimas]

@devinivy You definitely have a good point there! Maybe I'm overly concerned about the performance. My fears come from the fact that I just removed a useless model base class BaseModel from objection and saw a significant boost in my performance tests. But they are micro benchmarks that have little to do with real world applications.

Mixins actually seem like the way to go. I just did a full 180° turn here in a matter of minutes. Am I easily convinced or is @devinivy's idea just that great 😄 ?

[fl0w]

I'm not sure this is a matter of micro benchmarks though. A model is constructed for each fetched row. Add 3 or 4 added constructors (because of stacked mixins) and fetch 100 rows and well ... I haven't seen benchmarks, so I'm arguing from ignorance here - but I'm not optimistic. I would assume this does indeed add "worthy" performance hit for simply fetching data.

3 or 4 mixins (or plugins mind you) is a plausible scenario IMO.

[fl0w]

(Because as I understand it, a JS mixin is essentially an implicit super.constructor(), e.g. anonymous subclassing, correct?)

[koskimas]

Yeah, multiple levels of inheritance would cause a performance hit, but would it really be a common case to have 4 or more plugins that extend Model? And It's possible that the performance hit I measured was caused by code babel generates for implicit empty constructors. It's entirely possible that native classes work better and v8 is able to remove the dead implicit constructor code.

Prototype chains don't matter. v8 is able to completely remove those. I have once tested a 10000-level long prototype chain and it was just as fast as one level long one.

[wswoodruff]

I think mixins also bring the benefit of only getting the features you want in any given model. There's no requirement that all models in a project have that much inheritance, if your model needs that kind of performance then you can pull out some of the plugin code and implement it in your model definition. The mixins would be for convenience and speed of setup, but if you're really trying to dig in, you can subvert a long chain! Compose, compose

[fl0w]

I've been worried about the practical constructor (because I fetch large data sets at time), and what you're saying is that v8 should handle this pretty well. I did a super fast bench test which would suggest I'm wrong.

1 mixin x 65,000,776 ops/sec ±1.33% (84 runs sampled)
2 mixin x 62,377,499 ops/sec ±2.59% (81 runs sampled)
3 mixin x 59,460,265 ops/sec ±3.57% (75 runs sampled)
4 mixin x 65,967,845 ops/sec ±1.08% (83 runs sampled)

If anything, local OS scheduler is what adds the flux.

'use strict'

// npm i --save-dev benchmark microtime

const Benchmark = require('benchmark')
const suite = new Benchmark.Suite()

const MixinOne = (Base) => class extends Base {}
const MixinTwo = (Base) => class extends Base {}
const MixinThree = (Base) => class extends Base {}
const MixinFour = (Base) => class extends Base {}

class Base {}

class MyModelOne extends MixinOne(Base) {}
class MyModelTwo extends MixinTwo(MixinOne(Base)) {}
class MyModelThree extends MixinThree(MixinTwo(MixinOne(Base))) {}
class MyModelFour extends MixinFour(MixinThree(MixinTwo(MixinOne(Base)))) {}

let model
const n = 10000

suite.add('1 mixin', function () {
  model = undefined
  for (let i; i < n; i++)
    model = new MyModelOne()
}).add('2 mixin', function () {
  model = undefined
  for (let i; i < n; i++)
    model = new MyModelTwo()
}).add('3 mixin', function () {
  model = undefined
  for (let i; i < n; i++)
    model = new MyModelThree()
}).add('4 mixin', function () {
  model = undefined
  for (let i; i < n; i++)
    model = new MyModelFour()
}).on('complete', function() {
  console.log('Fastest is ' + this.filter('fastest').map('name'));
}).on('cycle', function(event) {
  console.log(String(event.target));
}).run({ 'async': true })

[fl0w]

I thought even this would have significant overhead. My bad, I withdraw my opinions. :)

[devinivy]

Wow I didn't expect such a turnaround, hahaa! Cheers! I like this approach. That's also a good idea, @WilliamSWoodruff– if one really needed to, there's likely a way you could compose a bunch of mixins with a nil class then hoist-up all the deep prototype properties to a single prototype.

@fl0w thanks for going through the trouble of testing that out! I certainly wasn't sure of the perf characteristics.

[koskimas]

Since you don't do anything with the result model v8 can remove the whole loop and your tests do nothing. What if you add a method foo() that returns a random number and then calculate and console log the sum of all the foo() results. That way v8 cannot remove the code.

[fl0w]

Oh, right ... I'm horrible at testingbenchmark. I'll do that. Hold my beer.

[koskimas]

I'm not sure if that's the case, but it's possible (not you being a horrible tester, but the thing I suggested before 😄). Also console.logging may be a bad idea. It will probably dominate the runtime. Maybe just return the sum? And thanks for doing this!

[fl0w]

Yea, I did

class Base {
  foo () {
    return Math.random()
  }
}

...
  let bar = 0
  for (let i; i < n; i++)
    bar = bar + new MyModelOne().foo()
  return bar
...

And see no significant performance difference. I might need to investigate how to actually benchmark mixins without v8 magically fixing the problems I try to create.

[koskimas]

It just may be that v8 is able to magically optimize away the useless constructors, which is an excellent thing!

[devinivy]

Ahhh V8, what a magical beast...

[fl0w]

Well, all things considered, mixins seems like what you're leaning towards @koskimas? Though, extending Model is not as powerful as first suggestion (monkey patching objection).

[fl0w]

So I disabled v8 optimisation using %NeverOptimizeFunction (which I assume catches class-sugar as well),
and a different try by wrapping construction and call to #foo() in a fn, i.e.

%NeverOptimizeFunction(Base)
%NeverOptimizeFunction(MixinOne)
// ... all mixins
%NeverOptimizeFunction(MyModelOne)
// ... all models
%NeverOptimizeFunction(constructOne)
// ... all wrappers
function constructOne {
  return (new MyModelOne().foo())
}

// suite goes below, here

And I still see no practical difference

1 mixin x 71,101,984 ops/sec ±3.87% (76 runs sampled)
2 mixin x 70,258,613 ops/sec ±4.50% (76 runs sampled)
3 mixin x 75,259,944 ops/sec ±1.44% (85 runs sampled)
4 mixin x 74,432,489 ops/sec ±1.20% (80 runs sampled)
5 mixin x 75,167,793 ops/sec ±1.28% (81 runs sampled)

Ps, I had to downgrade from node v7 to v6 as --allow-natives-syntax seems to have changed in v8@5.5. Still no difference.

[fl0w]

OMG, I caught my mistake. for (int i; i <n; i++) I never assigned i to 0.
Finding a minor (not sure how to evaluate the results yet) decrease per mixin.

https://gist.github.com/fl0w/932fd6dc2ef1c2964fdcb563f9a18479

// node v6.10.x

// 1
1 mixin x 154 ops/sec ±0.79% (73 runs sampled)
2 mixin x 117 ops/sec ±1.62% (70 runs sampled)
3 mixin x 83.11 ops/sec ±1.23% (67 runs sampled)
4 mixin x 72.57 ops/sec ±6.86% (58 runs sampled)
5 mixin x 69.06 ops/sec ±1.18% (67 runs sampled)

// 2
1 mixin x 157 ops/sec ±0.84% (75 runs sampled)
2 mixin x 112 ops/sec ±5.89% (68 runs sampled)
3 mixin x 93.77 ops/sec ±4.50% (65 runs sampled)
4 mixin x 78.99 ops/sec ±1.59% (65 runs sampled)
5 mixin x 68.02 ops/sec ±1.15% (66 runs sampled)

// 3
Just Base x 715 ops/sec ±1.37% (82 runs sampled)
0 mixin (extend Base) x 216 ops/sec ±5.02% (79 runs sampled)
1 mixin x 117 ops/sec ±8.76% (64 runs sampled)
2 mixin x 109 ops/sec ±4.48% (66 runs sampled)
3 mixin x 85.55 ops/sec ±7.00% (63 runs sampled)
4 mixin x 78.62 ops/sec ±0.88% (64 runs sampled)
5 mixin x 65.99 ops/sec ±1.00% (65 runs sampled)

// node v4.x and v7.7.x comes with a significant drop which should be related to v8
// hopefully also fixed in next major v8 performance release (v8 v5.7)

EDIT It looks like the performance boost you found by removing BaseModel/Model is justified by this benchmark. Worth considering before throwing mixins at this.

In the end, as far as I'm considered, I'd go with the old saying "composition over inheritance looks to be practical in node.js as well".

[wswoodruff]

@fl0w Thanks for doing these benchmarks, they're super important for this project especially! I think whatever conclusion it brings should be laid out in the docs when the 'plugins' feature is announced

[koskimas]

Now we are seeing some difference there! Hard to say how significant that is though. Even the five mixins case is still 6.8 million constructions per second. That's quite a bit of rows. I'll run my performance tests suite with different depths of native inheritance too. I'll post the results here.

[devinivy]

Fast enough for me! For those with extreme performance concerns there are probably tricky ways to collapse multiple mixins into a single mixin. I think it's well worth it for the ergonomics. And I know yall enjoy decorators, which will work with this approach too ;)

Also, strangely getting different results on runkit https://runkit.com/devinivy/58d13de053f6500014769d07

I did change the Math.random() to something else, but it didn't make a difference either way.

[koskimas]

I just ran a Person.query().eager("children.pets") query with a mock database (knex that creates the query, but returns the result from memory) with a result of 100 people, each having three children, each having 3 pets, so 400 Person instances and 1200 Animal instances. The execution time is 1.0X milliseconds with one mixin and with five mixins. So there is no measurable difference with native inheritance for this test.

With babel transpiled inheritance there was a 20 percent slowdown.

I think we can now conclude that performance is not an issue with mixins ∎

[fl0w]

@koskimas nicely done, there is however a noticeable performance hit in node v7.7.3 (using v8 v5.5), compared to node v6.10.x and v4.x.x (latest LTS). Unrelated, and I'll assume this gets corrected once v8 v5.7 hits shelves (secretly hoping for node v8 release, wouldn't count on it).

[koskimas]

@fl0w

Though, extending Model is not as powerful as first suggestion (monkey patching objection)

Most things in objection are classes, so there isn't much difference. Some things like the transaction function and global helpers like ref cannot be extended this way. Even relations like Model.HasOneRelation are classes, that's just not documented anywhere.

Anyway we are not forcing people to use mixins, just encouraging. They can still create monkey patching plugins if they want, nothing is stopping them.

[wswoodruff]

What's the next step with this?

[koskimas]

I'll write the plugin best practices section to the docs and add a place for a list of known plugins and packages using objection. I'll add the ones mentioned here to the list. More can be added through pull requests or issues. I'll also add a shortcut link to the plugin/package list from the README.

I will also write a contribution guide while I'm at it.

[wswoodruff]

Awesome! 👍

[ackerdev]

I can likely provide support for mixin style while maintaining backwards compatibility.

I would like to point out, though, that mixins can be a little awkward, especially as the list of mixins grows and if configuration is required.
It also directly competes with the decorators proposal which instead uses functions which alter the target class directly rather than creating anonymous subclasses. It's not entirely certain if/when it will make it into ECMAScript release, but the equivalent available today doesn't seem too bad either, and makes plugins future-ready. TypeScript also supports decorators today.

Mixin

// A little redundant...
class Foo extends softDelete(timestamp(Model)) {
  static get timestampAttributes() {
    return {
      create: 'createdOn',
      update: 'updatedOn'
    }
  }
}

// The syntax that gives me nightmares
class Bar extends softDelete(timestamp(Model, { create: 'createdOn', update: 'updatedOn' })) {}

Decoration

// ES6
class User extends Model {}
softDelete()(User);
timestamp({
  create: 'createdOn',
  update: 'updatedOn'
})(User);

// ES-next/TS Decorators
@softDelete()
@timestamp({
  create: 'createdOn',
  update: 'updatedOn'
})
class User extends Model {}

I'm not staunchly advocating for this here, just wanted to bring it up for consideration. I'll go with the flow on the final decision.

[devinivy]

I think there should be some solution available for those using "bare" node. Since the implementation of a decorator can be constructed from an existing mixin, I don't think there's any major conflict!

[koskimas]

@ackerdev Dammit, that's also a good point 😄

You don't have to use the nightmare inducing syntax though. You can do this:

let BaseClass = timestamp(Model, { 
  create: 'createdOn', 
  update: 'updatedOn' 
});
BaseClass = softDelete(BaseClass);

class Bar extends BaseClass {}

It's not that different from the ES6 decorator case.

@devinivy There is a solution for bare node: the ES6 example @ackerdev provided. Can a decorator be constructed from a mixin? For that to work, the extended class must be returned from the decorator and reassigned to the constructor variable. I don't think that's possible. I deed to read the spec more carefully.

What I love about the mixin approach is the user's choice and composability. We would also have all that with decorators.

I'm not saying I changed my mind again but, this is definitely something we should consider. I don't know why I didn't think of this, being a decorated decorator user.

[koskimas]

I was wrong, it is possible to construct a decorator from a mixin. In fact, mixins are decorators! From the spec:

@F("color")
@G
class Foo {
}

Desugaring (ES6)

var Foo = (function () {
  class Foo {
  }

  Foo = F("color")(Foo = G(Foo) || Foo) || Foo;
  return Foo;
})();

With the difference that if you do this:

const MyMixin = (BaseClass) {
  return class ClassyClass extends BaseClass {}
}

@MyMixin
class Person extends Model {

}

your Person class will show up as ClassyClass in debugger and console.log.

[ackerdev]

You still need a separate decorator-factory interface to support decorators if you provide configuration options, though, right?

Because the proposal shows decorators defined with options like so, a factory that returns a decorator function...

@F({ foo: 'bar' })
class Person extends Model {}

function F(options) {
  return function(target) {
    target._F = options;
  }
}

I'm fine with this, personally, though I think it should be added to the best practices if we want to go this route that plugin developers should try to support both interfaces in this manner, and suggest a common format to do so. Maybe something like:

function softDelete(target, options) {
  return class extends target {
    delete() {
      // do things
    }
  }
}

module.exports = {
  mixin: softDelete
  decorator: options => target => softDelete(target, options)
}

Although if you have a mixin that needs no configuration then both would be the same... so I'm not sure what is the best way about this.

[koskimas]

Nope, decorator can be any expression that evaluates to a function. So a function call that returns a function (your example) and just a function are both valid expressions.

The spec says:

  Decorator [Yield] :
   @ LeftHandSideExpression [?Yield]

[devinivy]

I personally don't think it should be up to a plugin author to have to think about decorators, mostly because not everyone uses them (they aren't part of nodejs today) and mixins are general enough– if someone wants to use their mixin as a decorator they can easily make that transformation. At worst I think the arguments might want to be flipped!

[koskimas]

At its simplest decorator is nothing but a function that takes a constructor function as its only parameter and optionally returns another (or the same) constructor. There is absolutely nothing they need to think about but that. It's even simpler than a Mixin function that needs to return a subclass.

[devinivy]

Sounds like a mixin– I think we're all good ;) :)

[koskimas]

To sum up a gitter conversation about interoperability of decorators and mixins: Mixins can be easily curried into decorators especially if options are given as the first argument.

class User extends Mixin1(mixin1Opts, Mixin2(mixin2Opts, Model)) {}

@curry(Mixin1)(mixin1Opts)
@curry(Mixin2)(mixin2Opts)
class User extends Model {

}

With this in mind, mixins seem like the best combination of "works now" and "future proof".

[koskimas]

One more thing: Maybe the options should be the second argument after all?

Using the timestamps mixin as an example: most of the time you would be ok with the default column names and wouldn't pass any options to it. I think it would be cleaner to allow the mixin to be used without configuration like this timestamps(Model) instead of something like timestamps(null, Model) or timestamps({}, Model). If the options come second, plugin developers don't need to handle the nasty "missing first argument" -case. Converting a "config second" -mixin into a decorator is only marginally more difficult and it's something that a helper like objection.toDecorator could handle. What do you think?

[koskimas]

After some additional research I ended up with mixins that only take one argument: the class to extend. Options should be passed using a factory function like this:

module.exports = (options) => {
  return (Model) => {
    return class Foo extends Model {

    };
  };
};

This keeps the mixins simple and supports the decorator case without modification. There is no commonly agreed specification for mixins. Taking only one argument keeps them compatible with libraries like mixwith. Also i think developers are familiar with the factory function pattern from things like expressjs middleware.

By the way, this article from the author of mixwith is worth reading.

I started the documentation work by creating a couple of example plugins. Please check them out and help me improve them.

https://github.com/Vincit/objection.js/tree/master/examples