Proposal for API changes

[rhys-vdw]

So I have been trying to think of ways that things could be improved. I've been using the library for about six months and I've hit many bugs and am aware of the weaker points of the API. Particularly the separation of concerns is problematic (especially when I was learning the interface).

I've never written any modern JS, but I've been trawling the docs after @tgriesser's example in #799. So I've taken the ideas I've had an think that they can be executed very effectively using the Babel toolset.

I've written up examples of how I think things could be implemented, and hopefully some of it resonates. Most of the changes are internal, so the API isn't drastically different.

Main things I want to address:

General separation of concerns

Query building and model state are too intertwined.

This, for instance, is very confusing to me: Model.forge({a: 'a', b: 'b'}).where(...).fetchAll(). I don't know what it does exactly, and would never write it, but I would prefer that it weren't possible.

Also, having state around queries seems wrong. The presence of resetQuery makes me uneasy that I might accidentally leave state in a model without realising it.

I would prefer very clear separation of concerns, where Model is a complex (but stateless) factory/resource management system, and model is a very lightweight model with no concept of queries, and no where.

So none of these are legal anymore in my proposal:

Model.forge().where(...)
new Model({id: 5}).fetchAll();
Model.forge({id: 4}).fetch();

Collections are not particularly useful

Pretty self explanatory and I think everyone agrees.

Queries are unexpected

Model.forge({id: 5}).someRelation().query() Gives a query that is only constrained to Model#tableName. This is confusing because you'd expect the library to be using query internally as you chain functions. It's also problematic (issues around count, and pagination, and just general flexibility.)

General refactor and modernization

The code is quite complex and would benefit from a refactor. Particularly in the relations area. In general I'd say that things are too stateful. There are also a few crufty abstractions like Sync that are very unclear how to use.

No nested saving, no bulk inserts.

Not even that hard a problem, unless I'm missing something. The main issue is that models are not aware of their relations. @tgriesser's suggestion of ES7 decorators comes in handy here. 😄

Too much configuration

The options convention is really useful, but seems bloated in some areas. Better to split this behaviour out into different well defined methods.

Anyway, I'm waffling on because I'm about to post lots of code and I wanted to give it some context. Sorry for the immense read!

[rhys-vdw]

So, firstly, for some context, here is the readme example as I'd have it. Whenever you see a Model used explicity, it could just as easily be bookshelf('Model') in the style of the model registry. I've basically tried to make the interface look as much like Tim's example yesterday as possible.

import {Model, relation, table} from 'bookshelf';

@table('users')
User extends Model {
  @relation
  static messages() { return this.hasMany(Posts); }
});

@table('messages')
Posts extends Model {
  @relation
  static tags() { return this.belongsToMany(Tag); }
}

@table('tags')
Tag extends Model {}

User.where('id', 1).withRelated('posts.tags').fetch()
  .then(user => console.log(user.related('posts').toJSON())
  .catch(error => console.error(error));

The @table decorator exists to add the table name directly to the model constructor (ie. Tag.tableName === 'tags').

[rhys-vdw]

Now the main thing I wanted to achieve with this design is to divorce the concept of chains from the actual model state. Because I want all 'command builder' chains to come off the constructor, they absolutely must not store state. A mutable constructor function seems bad. (I'm calling them 'command builders' to distinguish them from knex's 'query builder'.)

So, I came up with this concept of a ChainFactory class. I don't know how different it is from how lodash, knex do chaining, tbh, but this is unique in that it copies the static interface of the Model it starts on.

I'll start from the bottom:

// -- ChainFactory

// ChainFactory generates command chains that inherit from its static interface.
// Any call to `chain` will either create a new chain, or continue an existing
// one.
//
// The rule here is that every chainable method calls `chain` internally
// to get an instance of "itself". This ensure that it will make sense when
// bound to the ChainFactory constructor, and also when it's intantiated. When
// instantiated `chainInstance.chain` is overridden to return itself.
//
// This obligation to call `.chain()` is hidden from consumers of Bookshelf, as
// they can make their own chaining functions by calling `.query()` or `.where()`
// as they can now.
class ChainFactory {
  static chain() {

    // Create new object with this constructor as its prototype. This will give
    // the `chain` the static properties Class.*
    let chain = Object.create(this);

    // Remove any static functions we don't want to chain. eg. `forge`.
    // We just override their name on the current object, shadowing the super
    // version.
    //
    // __notChainable is assigned by the `@doNotChain` decorator.
    for (property in this) {
      if (property.__notChainable) {
        chain[property] = null;
      }
    }

    // Now override the `prototype.chain` (ie. `ChainFactory.chain`), with a
    // function that returns itself. This way any chained calls
    // will not produce new instances.
    chain.chain = function () { return this; };

    // Allow state to be assigned here by inheriting classes.
    if (this.initializeChain) this.initializeChain(chain);

    // Return the new chain instance.
    return chain;
  }
}


// -- `@doNotChain` static method decoration

// Tacks on a little flag to methods that we don't want to reveal in chains.
//
//     class MyThingo extends ChainFactory {
//       @doNotChain
//       someStaticFunction() {
//         // ...
//       }
//     }
//
function doNotChain() {
  return function decorator(staticFunction, name, descriptor) {
    staticFunction.__notChainable = true;
  }
}

So, just say you have Model extends ChainFactory, the object returned from Model.chain() is a new instance whose prototype is Model itself (not Model.prototype, which is what you'd get from new Model()). This means that it inherits the static interface. Because there are static methods that don't make sense in a chain, you can decorate them with @doNotChain to tell the chain factory to hide them.

This doesn't make a lot of sense in isolation so I'll move on to Model now.

[rhys-vdw]

So, he is the (fairly large) Model class that I've concocted.

// -- Model, based on existing model, but different.

@staticAlias('q', 'query');
class Model extends ChainFactory {

  // Build a new model instance. Model.forge() is preferred.
  //
  //     Model.forge({id: 5});
  //     // same as
  //     new Model({id: 5});
  //
  // This is also overridden, however, to initiate a transacted save.
  //
  //     Model.transacting(trx).where(...
  // 
  // is the same as:
  //
  //     Model(trx).where(...
  //
  constructor(attributes) {

    // No need to call `super` here, we're just inheriting static properties (amazing!).

    // Check if this is not being called as a constructor. Gives us the
    // transaction shorthand above.
    //
    // Much terser than `{transacting: trx}`, but still fairly clear in the
    // context of a transaction callback.
    if (arguments.callee !== this.constructor) {
      // If there are no arguments we return a command chain bound to a
      // transaction.
      let transaction = attributes;
      return this.transacting(transaction);
    }

    // Set initial attribute state.
    this.attributes = new Map();
    this.set(attributes);

    // Call overridden initializer.
    if (this.initialize) this.initialize();
  }

  get id() {
    if (Array.isArray(idAttribute)) {
      return idAttributes.map(attr => this.attributes[attr]);
    }
    return this.attributes[idAttribute];
  }

  isNew() {
    return _.isEmpty(this.id);
  }

  // Classic attribute mutator.
  set(attributeName, value) {
    _.isString(attributeName) {
      this.attributes.set(attributeName, value);
    } else {
      let attributes = attributeName;
      Object.keys(attributes).forEach((key) =>
        this.attributes.set(key, attributes[key])
      );
    }
  }

  // Classic attribute accessor.
  get(key) {
    return this.attributes.get(key);
  }

  // A few passthroughs here, for convenience.
  load(related, {transacting}) {
    return this.constructor(transacting).withRelated(related).load(this);
  }
  save({withRelated, transacting}) {
    if (this.isNew()) {
      this.create.apply(this, arguments);
    } else {
      this.patch.apply(this, arguments);
    }
  }
  patch({withRelated, transacting}) {
    return this.constructor(transacting).withRelated(related).patch(this);
  }
  create({withRelated, transacting}) {
    return this.constructor(transacting).withRelated(related).create(this);
  }

  // `refresh` replaces `fetch`. `fetch` only exists to terminate chains. You
  // can only `refresh` models. (I kind of prefer `renew` here because it sounds
  // like re-borrowing a book from a library. Thematic heh.)
  refresh({transacting}) {
    let attributes = this.isNew()
      :  this.attributes
      ?  _.pick(this.attributes, this.idAttribute);

    return this.constructor(transacting).where(attributes).fetch()
  }

  // -- Static non-chained members --

  // Model.load(engine, 'carriages')
  // // or...
  // Model.load(users, 'accounts', 'messages')
  //
  // Doesn't support nested loading.
  @doNotChain;
  static load(target, ...related) {

    // Accept array input.
    if (_.isArray(related[0])) related = related[0];

    // Bulk load each relation type.
    return Promise.all(related.map(relation => {
      let rel = this.relation(relation);
      return rel.attach(target);
    });
  }

  @doNotChain;
  static forge(attributes) {
    return new this(attributes);
  }

  // -- Chain members --

  // Intialize the chain instance.
  @doNotChain;
  static initializeChain(chain) {

    // `this` here is the constructor, as it's a static function.  Note,
    // `tableName` is attached statically to the class constructor.
    chain._ModelConstructor = this;
    chain._queryBuilder = knex(this.tableName); 
    chain._withRelated = new Set();
    chain._returnSingle = true;
    chain._columns = undefined;
    chain._where = new Map();
  }

  // Set this chain to return all rows when `fetch` is called.
  static all() {
    let chain = this.chain();
    chain._returnSingle = false;
    return chain;
  }

  // Set this chain to return a single row when `fetch` is called.
  static one() {
    let chain = this.chain();
    chain._returnSingle = true;
    return chain;
  }

  // Add relations to be retrieved with the query.
  // Could just be called `with`.
  //
  //   House.where({id: 5}).with([rooms, occupants]).fetch();
  //
  static withRelated(relations) {
    let chain = this.chain()
    relations = Array.isArray(relations) ? relations : [relations];
    relations.forEach(relation =>
      chain._withRelated.add(relation);
    );
    return chain;
  }

  // How about using the following instead of the `object` syntax?
  //
  //     Model.q('where', 'id', '<', 10).q('where', 'id', >, 2).all().fetch().then(//...
  //
  static query(method, ...args) {
    let chain = this.chain();
    let query = chain._queryBuilder;

    // Handle 'whereIn', 'id', [0,2,3] type syntax.
    if (_.isString(method) {
      query[method].apply(query, args)
      return chain;

    // Handle callback syntax.
    } else if (_.isFunction(method)) {
      method.call(query, query, knex);
      return chain;

    // Handle terminating chain when no arguments are passed.
    } else if (_.isUndefined(method)) {
      return query;

    // Tell user if they made a mistake.
    } else {
      throw new Error('unexpected argument');
    }
  }

  static limit(limit) {
    this.query('limit', limit);
  }

  static offset(offset) {
    this.query('offset', offset);
  }

  // Set the current transaction.
  // Actually would like to use this shortcut as well:
  //
  //     Model(transaction).where('id', 5)...
  //
  // This allows `Model(null)` too, so you can write functions that are
  // transaction agnostic.
  //
  static transacting(transaction) {
    if (transaction) {
      return this.query('transacting', transaction);
    }
    return this.chain();
  }

  // Just the hash argument here.
  static where(attributes) {

    // Do all the prefixing and formatting here so that the consumer doesn't
    // have to. Also helps with DRY in Relations.
    //
    let formatted = this.prefixKeys(_.mapKeys(attributes, this._formatKey));
    return this.query('where', formatted);
  }

  // `fetch`. Now you can't call it on instances (see `renew`. It terminates a
  // command chain and builds a query based on the state it accrued during its
  // lifespan.
  //
  static fetch() {

    let single  = this._returnSingle,
        columns = this._columns,
        related = this._withRelated,
        Model   = this._ModelConstructor;

    // Now construct query. We just discarded the last reference to the chain
    // by not providing `query` with arguments.
    let qb = this.chain().query();

    if (single) qb.limit(1);

    // NOTE: `_this._columns` is `undefined` by default, so becomes '*'.
    return qb.select(columns) 
      .get('rows')
      .map(record => Model.forge(_.mapKeys(record, this.parseKey))
      .then((models) => _ModelConstructor.load(models, related))
  }

  static relation(relationName) {

    // Ensure we have a relations object.
    let relations = this._relations || this._relations = {};

    // Do we already have a cached version of this relation?
    if (relations[relationName]) {

      // Yes, return it.
      return relations[relationName];
    } else if (this[relationName].__isRelation) {

      // No, but we can make one.
      return relations[relationName] = this[relationName]();

      // Friendly errors.
    } else if (this[relationName]) {
      throw new Error(`
        There is a static function called ${relationName}, but it has not
        been marked as a relation with the `@relation` attribute.
      `);
    }
    throw new Error(`Invalid relation name: ${relationName}`);
  }

  // -- Helpers.

  static parseKey(key) {
    return key;
  }

  static formatKey(key) {
    return key;
  }

  static prefixKeys(attributes) {
    return _.mapKeys(attributes, (value, key) => `${this.tableName}.key`)
  }
}

[rhys-vdw]

Okay, so now for some explanation...

So, the important thing to note is that Babel's extend performs not only the normal inheritance you'd expect...

class Child extends Parent { //...

// is equivalent to:
Child.prototype = Parent.prototype;
Child.__proto__ = Parent;

This means that every time you extend, you get to take your entire static interface with you. This is the first language I've been able to do this in, so pretty cool.

Chain has all the static functions of Model (besides the ones decorated with @doNotChain), but their this value is pointing at the chain instance instead of the static model. To make this work, both Model and chain need to conform to the same interface - they both have a method called chain(), but it behaves differently for each.

// creating a new chain: internally `where` calls `Model.chain` which is the chain factory method.
// These are two separate instances:
let chain1 = Model.where();
let chain2 = Model.where();

// However, because we inherit the static interface, they are easily modified by custom class definitions.
@table('songs');
class Song extends Model {
  static favourites() {
    return this.query('listen_count', '>', 100); // calls `chain` internally.
  }
}

@table('musicians');
class Music extends Model {
  @relation
  static songs() { return this.hasMany(Song); }
}

// Just works. (theoretically, ha)
Music.related('songs')
  .favourites()
  .whereIn('id', [0,1,2,3,4,5])
  .query('artist_name', 'ilike', 'devo')
  .query().count()
  .then(result => console.log('you learned: ', result));

[rhys-vdw]

So, something I noticed is true in the current code base, is that Relation is essentially immutable after construction. This means that I was safely able to copy it by reference when I was restoring Model#clone the other week. Of course nothing's really immutable in JS, but the API is currently designed so that you never interact directly with the Relation object.

So why bother instantiating these per object? You only need one per class. The basic idea is that the supplied relation definitions, eg

@relation
static function acquaintances { return this.hasMany(Acquaintance).through(Friend); }

...are considered to be factories for Relation models. (Much as they are now, but this time attached to the constructor instead of the instance, and only executed once).

The additional advantage of having these @relation attributes is that we can straight away know exactly which relations exist on a model, without any initialisation.

So, this makes these types of things much simpler:

let person = {
  name: 'Joe',
  friends: [
    {name: 'Tom'}, {name: 'Dick'}, {name: 'Harry'}
  ]
}

Person.withRelated('friends').save(person)
.then(joe => console.log('joe is friends with ', _.pluck(joe.toJSON().friends, 'name').join(', '));

This is how I do the same thing now:

var person = // as above
var friends = person.friends;
Person.forge(_.omit(person, _.isObject)).save().tap(function(person) {
  // Note constructing lots of Model instances here, would no longer be necessary.
  return person.related('friends').add(friends).invokeThen('save');
}).then(function (joe) {
  console.log('joe is friends with ', _.pluck(joe.toJSON().friends, 'name').join(', ');
});

Now Person already knows what relations a Person can have. This also makes the options withRelated: true possible, which would just store all relations. You could have a model with 12 different relations and your code would not get any larger (unlike the last example which would almost double in size). But anyway, I'm going to put the final touches on my Relation classes and post them here.

[rhys-vdw]

But first I'll mention transactions.

This is my proposed API:

bookshelf.transaction(transacting => {

  // `withRelated()` without arguments defaults to saving all known relations.
  return Model(transacting).withRelated().save(data).then(model => { //...

  // Or we can do the equivalent from a `model` instance (the old way) like so:
  return Model.forge(data).save({withRelated: true, transacting}).then(model => { //...
});

Basically (shown in the massive Model definition above), the constructor is able to check if it's being called with new or not, and decide to return a chain with the query set to a transaction. This is purely for brevity.

[tgriesser]

It might be easier to create some of these as gists so we can discuss/update/fork with actual diffs, but a few notes:

1. Proposal for API changes #803 (comment) This has not accounted for actually making a connection anywhere. The idea behind the example I posted was that the connection to the underlying database is separated from any model definition.
2. Agreed, forge is going to go away entirely, I'm thinking that bookshelf(modelRef) will return a new model instance and static methods will go away for the most part, unless you want to use them for something custom but they won't be nearly as useful or necessary as they are now
3. The chaining isn't the difficult part, it's figuring out how to keep an efficient internal registry to be able to follow the unit of work pattern and deal with multiple objects mapping to the same row
4. Relations should be statically defined, and I'm thinking they should even be defined outside of the bookshelf layer entirely at a separate layer which is consumed/referenced by models - these can be decorated into custom relations via methods, but at the core the entire schema should be statically analyzable

I just pushed a branch next where I'm also sketching out some ideas for some apis / structure. Feel free to take a look

[rhys-vdw]

Okay, stuff to think about certainly. I'll have a look through your branch.

1. I must admit I'm not entirely clear on your meaning here. Do you mean connecting to the database? Or connecting models by a conceptual relationship? (btw, that was just the readme example with changed syntax)
2. Right, my main concern is not forge, but that model data and queries are all jammed into the one object abstraction, and they aren't necessarily related. Commands like where, select etc. I think that it's overloading the instance abstraction - hence why I went to the static interface.
3. I'll read up about it. The intention would be to automatically update multiple models for the same item on insert/update?
4. Yeah, that's a good idea. When you say 'the entire schema', do you just mean models and relations, or columns/types too?

[tgriesser]

1. Do you mean connecting to the database?

Yep, in your example:

User.where('id', 1).withRelated('posts.tags').fetch()

wouldn't work, because the User constructor doesn't have any knowledge of the db connection, that's why in my example you'd adapted from it'd be something like:

var db = Bookshelf(knex)
db(User).where('id', 1).withRelated(...

or

db('users').where(...

Yeah, that's a good idea. When you say 'the entire schema', do you just mean models and relations, or columns/types too?

Columns/types/potentially indexes as well, and while this info might not be required there's a whole lot more you can do / optimize (and less you'd need to type) when you know everything about the data layer looks like upfront, though I don't think this info should be defined on the model... it should be passed into the initialization of the Bookshelf/Knex libs.

In fact, you might not need™ to define "bookshelf models" unless you need to hook in for validation/serialization/instrumentation purposes, they could be created automatically based on the internal knowledge of the schema.

I'd also like to ensure there's appropriate helpers in knex to fetch the schema info from the DB so you don't need to write them all out by hand if you'd prefer the DB itself be the source of the schema.

[rhys-vdw]

...the User constructor doesn't have any knowledge of the db connection

Neither the constructor, not the models have a reference to knex attached. Model has knex in scope, and creates a new knex QueryBuilder whenever a constructor chain is called. (see Model#initializeChain above for specifics.) This is to simplify design around query state (since there is none attached to instances). And also why I suggested that asPartial might not be necessary, as the query only lives for the lifespan of the chain.

{Model} = Bookshelf(knex);

@table('users')
User extends Model { ... }

// `User` has no knowledge of connection/knex
User.where('id', 1).withRelated('posts.tags').fetch().then(user => //...
    |<-----        knex instance lifespan      ----->|

// `user` also doesn't have any knowledge of db or knex. It just knows it can
// call `super.save(this)` to save itself.

In fact, you might not need™ to define "bookshelf models"

Well, you may not need to, but generally I like to add methods to my models for use within my app. My models tend to have a lot of business logic or helper methods to them. eg. User.resetPassword() User.allowedAccessTo(resource). There would still need to be some way to define functions like this.

The kinds model extensions I use (as a consumer of Bookshelf 0.8) can be grouped into three categories: instance methods (set, get), static methods (forge), and chainable methods (query, any relation method, where).

What I would like to see is a clean, accessible way of defining and codifying those concepts and separating their APIs and state. But also allowing new models to be extended easily for use in application logic. The class syntax or Model.extend both seem like well understood conventions for defining objects, and I think the constructor chain thing is a nice way of encapsulating query state.

What I really don't want is model instances to have any knowledge of a connection, or a knex instance. Just to be simple data containers with get, set, attributes, and a few convenience methods:

  patch({withRelated, transacting}) {
    return this.constructor(transacting).withRelated(related).patch(this);
  }
  create({withRelated, transacting}) {
    return this.constructor(transacting).withRelated(related).create(this);
  }
// ...

Sorry if I'm banging on about this, I just want to make sure that you understand what I'm getting at. I feel as though I haven't fully grasped your vision presently.

Columns/types/potentially indexes as well

That's good to hear. I was under the impression that you didn't want that level of coupling between schema and ORM.

Ultimately, I'd probably prefer to tie the schema to the models, rather than read the schema from the DB. Then use a tool to generate knex migrations. That seems pretty far off at this stage though.

[tgriesser]

Model has knex in scope, and creates a new knex QueryBuilder whenever a constructor chain is called.

Yeah, this is what I'd like to get away from even further. Model should not be required to have knex in scope at construction time. You should be able to

import {Model} from 'bookshelf' // not bookshelf(knex)

class User extends Model {

  // ...

}

Well, you may not need to, but generally I like to add methods to my models for use within my app.

Yeah, for some, sure. I have an application with ~30ish tables and only about 4 of them need actual "business logic" methods defined... the rest of it is boilerplate that disappears when you know the schema.

And yeah, it'd basically mean that any static methods would just be defined directly on prototype

Ultimately, I'd probably prefer to tie the schema to the models, rather than read the schema from the DB. Then use a tool to generate knex migrations. That seems pretty far off at this stage though.

Yeah that's sort of the idea, that you could potentially do both.

[rhys-vdw]

Model should not be required to have knex in scope at construction time.

Right. The one advantage I can really see to this is the ability to break models into different modules without having to pass a bookshelf instance to them. Is that the idea? In my current project I've separated my models and bookshelf initialization into a separate module, but this means doing this:

# 'server' module
orm = require('models')(config)

MyModel = orm.model('MyModel')

# 'models' module
module.exports = (config) ->
  knex = require('knex')(config.knex)
  bookshelf = require('bookshelf')(knex)

  getFiles('./models').forEach (file) ->
    require(file)(bookshelf, config)

# ./models/*
module.exports (bookshelf) ->
  bookshelf.model 'ModelName', bookshelf.Model.extend {
    # ...
  }

It's kind annoying. It would be a lot cleaner if I could do this (which you appear to be suggesting):

# 'my-models' module
 module.exports {
  User = require('./models/user')
  Account = require('./models/account')
  #...
}

# ./models/*

bookshelf = require('bookshelf')

module.exports = bookshelf.Model.extend {
  # ...
}

# 'server' module 
knex = knex(config)
bookshelf = bookshelf(knex)
bookshelf.registerModels(require('my-models'))

That said, all bookshelf() needs to do is this:

bookshelf = function(modelName) {
  Model = this.modelCache[modelName];

  // In my approach like this. In yours you'd extend the prototype.
  Model.createQueryBuilder = function() {
    return new knex(Model.tableName);
  }
}

bookshelf.registerModels = function(models) {
  let cache = bookshelf.modelCache;
  _.extend(cache, models);
}

So it's really not a very big change. In fact, I wonder how hard it would be to get a change like this going in master presently...

the rest of it is boilerplate that disappears when you know the schema.

I can certainly see the advantage to that approach. Implicit relations would be very cool, but they would still require names for use. For instance, table A could have two different has-many relations to table B, (ie. table B has two FK columns referencing table A). So this would be ambiguous:

// Just assuming that you'd use table names instead of
// model names when they're implicitly defined.
bookshelf('table_a').related('table_b').fetchAll()

Or perhaps that's fine to have as a kind of default setting, and you can upgrade to class definitions when you feel a concept is sufficiently complex, the above case being an example of that?

[tgriesser]

So it's really not a very big change. In fact, I wonder how hard it would be to get a change like this going in master presently...

It's a bit bigger than that, because there's a lot more that I'd like to accomplish with this change. Also mutating the model constructor like that sort of defeats the purpose, in mine you wouldn't extend the prototype, you'd instantiate a new Model passing the connection in the constructor.... no more using "new"

Also if you have multiple connections accessing the same models, you can't mutate them, you'd need to extend and cache

I can certainly see the advantage to that approach. Implicit relations would be very cool, but they would still require names for use. For instance, table A could have two different has-many relations to table B, (ie. table B has two FK columns referencing table A). So this would be ambiguous:

Presumably the relations would come with pre-defined names. Haven't fully determined the syntax.

[rhys-vdw]

Also if you have multiple connections accessing the same models, you can't mutate them, you'd need to extend and cache.

Right. I'm with you now.

I'll read through the next branch and keep iterating on this concept.

Presumably the relations would come with pre-defined names.

Thoughts on syntax:

You could generate names based on column names, using similar assumptions that Bookshelf uses currently. So, say there is a table called people, with two FKs to people called friend_id, and another called family_member_id, you could generate a model called Person, and relations called friends and familyMembers. (You'd need a good pluralization library for the people to Person conversion, but it would be cool.)

So, Person could be a 'default model', with no extra functionality. The model could be instantiated lazily (ie. first time you request Person model, it looks through the schema for a table called people).

If you've already called bookshelf.registerModel('Person', MyPersonModel) then it will extend the model with whatever extra functionality you like, on top of the default relations. If you don't follow the convention of relation_name_id you would be required to add your own relations here (or potentially use a more. So, you get all the benefits of automatic relations, with flexibility to extend at the cost of more verbose code.

// entry point to program.
knex = require('knex')(/** config **/)
bookshelf = require('bookshelf')(knex);

// No definition required, bookshelf intuits (and caches) relation `friends` when it
// first initializes `Person` internally, via schema analysis.
bookshelf('Person').friends().where('name', 'ilike', 'Joe%').fetchAll()

// Now just say your 'friends' relation doesn't follow the required convention, you could do either of these:
{Model, HasMany} = bookshelf;

// cbf defining a model:
bookshelf('Person').addRelation(HasMany, 'friends', 'people', 'friendFK');
bookshelf('Person').friends().where('name', 'ilike', 'Joe%').fetchAll()

// or with a model (not necessarily using decorators and stuff, I'm not even sure
// if they're a good idea, but you get the point)
@tableName 'people'
Person extends Model {
  sayHello() {
    console.log(`Hello, ${@get('name')}!`);
  }

  // not sure if this attribute or static is necessary now, but via some method.
  @relation
  static friends() { return new HasMany(this, 'people', 'friendFK') };
}

// Register that model.
bookshelf.registerModel('Person', Person);

// Use original syntax. Only this time you get your decorated model.
bookshelf('Person').friends().where('name', 'ilike', 'Joe%').fetchAll()

// Can still access `familyMembers` because it *did* follow bookshelf conventions.
bookshelf('Person').familyMembers()...

I realized while writing this that only one side of the relationship has a foreign key though, so it's not going to be able to generate names via the column name for everything (in fact the above example a self-referential edge case, it would usually only work for belongsTo and belongsToMany relationships, I can rewrite it later). Most of the time the FK is just the name of the table, singularized with _id attached. You could default the relation names on the other side with the relation's model name.

[rhys-vdw]

Also, I'm sure you're aware of this, but having the models unaware of connections unify the transaction and normal syntax, which is really cool.

bookshelf('Model')...
bookshelf.transaction((transaction) {
  transaction('Model')...
});

But also another reason for not storing the queryBuilder in the model instance itself, as it might be tempting to take the model instance and use it outside of the transaction cb (which would not work). Perhaps the result of fetch should just be a simple object state (set, get, changed, etc.) without any save, load function at all, so you have to do this:

bookshelf.transaction((t) {
  return t('MyModel').where(...).fetch()
    .then(m => t('MyModel').load(m, 'relationName')
    .then(m => // ...
}.then(modelObject => {
  // reusing model instance from trx, but it doesn't matter because all the connection info
  // is coming from `bookshelf.
  return bookshelf(`MyModel`).save(modelObject);
})

Hm... Certainly not as readable as model.save()...

[skysteve]

Hey, after a comment on another issue I thought I'd raise my support for collections here rather confusing another thread...

First off I've only been using bookshelf for about 2-3 weeks so please tell me if I'm missing something. I do however come from a backbone background of about 2-3 years.

I find collections great for keeping models clean to handle instance specific tasks and collections great for handling multiple instance specific tasks.I don't have a collection for every model in my project, so I can see how they may be viewed as pointless in some applications but where I do have them I find them very useful.

At the moment I'm mostly using collections to load a group of models with a given query or a cache for model's I've already fetched and then retrieving them using collection.find rather than having to implement this manually using an array. And in some places I'm using other underscore methods like filter which again saves implementing it manually. But some collections also have helper methods on them which save me re-writing the code or confusing the model.

e.g. (if you need some context let me know)

getAccountToFetch : function () {
            //try to find the account we were last loading - it'll have a cursor that's not 0
            var current = this.find(function (model) {
                var cursor = model.get('cursor') || null;

                return (!_.isNull(cursor) && cursor !== '0');
            });

            //if we have one let's carry on
            if (current) {
                return current;
            }

            //otherwise get the account we refreshed the longest ago
            return this.min(function (model) {
                return model.get('lastFetched');
            });
        }

That said, collections aren't perfect, for example I would argue that the model.fetchAll method and collection.fetchOne are very confusing/pointless as model.fetchAll is just collection.fetch and collection.fetchOne is just model.fetch. And there's no way to save everything in a collection (that I can find) which means you end up having to do something like

 collection.each(function (model) {
          return model.save();
});

[kevinob11]

@rhys-vdw Is this conversation on-going somewhere? Is most of the work just being done on the next branch? I've made a big bet on a project replacing waterline in sailsjs with bookshelf. I'm excited about the next version and love some of the ideas being discussed here. I'd love to be able to know where to look to get an idea of what progress / changes are going into the next version.

[jordansexton]

Last public commit to next was June 17 (282abe5). I would also like to know if Bookshelf is under active development.

[rhys-vdw]

@kevinob11 @jordansexton Bookshelf is not currently under active development.

[Ben2HellAndBack]

@kevinob11 @jordansexton I'd suggest objection - https://github.com/Vincit/objection.js/ It also plays nicley with es6 & 7.

[kevinob11]

@B3njamin not sure I'm ready to jump ship right away after a couple of weeks of integrating this with sailsjs, objection looks young, but does look pretty nice. @rhys-vdw bookshelf isn't going to be abandoned correct? What is current sentiment among maintainers?

[arnold-almeida]

@rhys-vdw Abandoned?

[vellotis]

@arnold-almeida
Maintained by me and @rhys-vdw.

[fcarreiro]

Hi @vellotis @rhys-vdw ! I'm wondering about the status of Bookshelf. I'm using it for a new project and I really like it. I saw some ongoing discussions (in 2015/2016) about the next version, and using ES6/7. Are there plans to continue development of Bookshelf or... should I consider moving to another ORM? 😟

[jordansexton]

@fcarreiro Bookshelf and Knex are stable, flexible, full featured, well tested, and used in production by many individuals and organizations. In addition, there are many plugins still being developed for it. By all means, consider Bookshelf (or any other ORMs you might evaluate) as you would any other library: on its merits.

You're essentially asking the library maintainers to project their expected effort in the future, and thus bear responsibility for your decision to use (or not use) Bookshelf now. As a counterpoint, if Bookshelf currently seems unmaintained/is missing features/doesn't use ES6/7, and that presents a problem for your use of it right away, what would really cause that perception to change?

Use Bookshelf, or "consider moving to another ORM" if you think that's best, but it seems to me that evaluating the historical and current development on the project per the commit history, Pulse, and Graphs on Github is a much better idea than deciding based on a reply to a comment on a 2 year old issue.

Per @rhys-vdw's comment above, Bookshelf isn't under active development. To me, that's not a huge problem, because of all the existing great things about it, but you can read the above thread and look at the commit history to assess the state of the project.

[fcarreiro]

Hi @jordansexton, thanks for the answer!

I have used Bookshelf, Sequelize and Waterline and also compared all the graphs you mention, and I ended up choosing Bookshelf. I think that it provides most of the things I need. My question was targeted at understanding if the current maintainers are planning to continue working on "new features/versions" or, for example, if they will mostly focus on bugfixes and minimal maintenance. In no way do I expect them to estimate their efforts nor do I expect them to choose an ORM for me.

What could represent a problem for me is the amount of unprocessed issues and pull requests. I wanted to know that if I was to find some problem and/or to propose a pull request, that it would be taken into account. Also, that the dependencies are regularly updated, etc. I don't expect the maintainers to do all the work (for free). I'd like to contribute myself but I've had the experience in other projects that sometimes no matter how much effort you do, the project stays stale.

Edit: regarding the stability, I certainly couldn't guess it from the versioning numbers, so I asked! ;)