bendrucker / ama

Ask me questions about building web applications
MIT License
6 stars 1 forks source link

Bookshelf registry #2

Closed bsiddiqui closed 9 years ago

bsiddiqui commented 9 years ago

Hey Ben - the Bookshelf plugin registry is helpful for avoiding circular dependencies by making models available as strings when defining relations.

2 questions: 1) What's the best way to register models? Do you do it at the top of each file like in the example or do you have a central place where you register?

2) What's the best way to refer to external models in model methods?

var bookshelf = require('../config/db');
require('./userModel');

var User = Db.model('User') // undefined

module.exports = Db.model('Channel', {

   tableName: 'channels',

   members: function () {
      return this.belongsToMany('User')
      .through('UserChannel'); // registry is helpful here
   },

   addMember = function (userId) {
      var self = this;

      return User.forge({ id: id }) //cannot call 'forge' on undefined
      .fetch()
      .then(function (user) {
         return self.members().attach(user);
      })
      .then(function (user) {
         return user;
      });
   })
});

Should I instead not define the User var and just refer to the User model with bookshelf.model('User')? That seems to work but I'd like to know how you recommend setting this up

Thanks!

bendrucker commented 9 years ago
  1. The point is just that someone need to call require, or else the file itself will never be evaluated. For the Valet.io API, I accomplish this by exposing the model via server.expose in its plugin and then loading the plugin.
  2. In your particular case, you don't need the User model at all. You can also use this.members().create. And even if you really just do need the User constructor, it's this.members().model.

On a semi-related note, you shouldn't be using self = this here. You can use Bluebird's bind and get both more readable/portable code and better perf. Function.prototype.bind is still better than closures from a readability/clarity point of view, but worse than closures when it comes to perf. Save it for cases where you're not calling the method frequently (i.e. not production code within a model).

bsiddiqui commented 9 years ago

thanks Ben!

bsiddiqui commented 9 years ago

Actually @bendrucker how can you use this.members().create? Do you mean this.members.attach?

this.members().create seems like it would be used to create a new User and add that User to the UserChannel collection

bendrucker commented 9 years ago

It automatically sets up the join row for you:

https://github.com/tgriesser/bookshelf/blob/master/lib/collection.js#L88-L110

bsiddiqui commented 9 years ago

Ok a couple clarifications - sounds like you're suggesting I do something like this:

   addMember = function (userId) {
      return this().members().create({ id: userId})
      .then(function (member) {
         return member;
      });
   })

if I understand bookshelf's create method, it looks like it would expect attributes for the model to add to the collection. So it would take user attributes, add it to the User table, and then setup the join row. If you have an already existing User and don't want to create a new one, would you still use create? Maybe I'm missing something

Also, do you have each model setup as a plugin? You mentioned using server.expose and loading the plugin?

bendrucker commented 9 years ago

So it would take user attributes, add it to the User table, and then setup the join row.

Right

If you have an already existing User and don't want to create a new one, would you still use create?

You wouldn't. Previous answer was basically wrong. create is helpful for eliminating the need to explicitly grab a reference to a separate model, but not appropriate for this case.

You can always pass the id directly to attach and not an actual model when you don't need the return value. In this case it looks like you need the call to resolve the user (though the last noop block should be eliminated).

You can definitely just eval bookshelf.model('User') inside the method and be fine that way. Or this.members().model. Both are the same.

bsiddiqui commented 9 years ago

Thanks!