Skip to content
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

db.load vs orm.express #291

Closed
colegleason opened this issue Aug 8, 2013 · 9 comments
Closed

db.load vs orm.express #291

colegleason opened this issue Aug 8, 2013 · 9 comments

Comments

@colegleason
Copy link

I have a single models.js module that I'd like to load in both Express request handlers and non-Express code. It would be nice if the function definitions for these two cases could be similar so that the models.exports function could be written once well.

Express expects a function with db and models as an argument:

define: function (db, models) {
    models.person = db.define("person", { ... });
}

On the other hand, db.load wants to pass just a callback:

db.load("./models", function (err) {
    // loaded!
    var Person = db.models.person;
    var Pet    = db.models.pet;
});
@dxg
Copy link
Collaborator

dxg commented Aug 11, 2013

+1 this needs to be solved as it's causing confusion.

The whole idea of integrating model definitions into express has bugged me from the begining. People get really confused when they need to eg have a "sync" task runnable from the command line.

Personally, I use something like this:

// database.js

var connection = null;

function setup(db) {
  var User  = db.define('user', ...);
  var Email = db.define('email' ...);
  Email.hasOne('user', User, ...);
}

module.exports = function (cb) {
  if (connection) return connection;

  var opts = {
    host     : 'localhost',
    database : 'blah',
    protocol : 'mysql',
    port     : '3306',
    query    : { pool: true }
  };

  orm.connect(opts, function (err, db) {
    if (err) return cb(err);

    connections = db;
    setup(db);

    cb(null, db);
  });  
};

// middleware

var database = require('./database');

app.use(function (req, res, next) {
  database(function (err, db) {
    if (err) return res.send(500, "cannot connect ot database");

    req.db = db;
    req.models = db.models;

    next();
  });
});

I omit the built in methods of loading altogether as they don't work how I'd like.
We could probably integrate db.load into the above, and provide a simple middleware to add req.db and req.models.
I'm also planning to add a simple example app with express, a sync task, and some basic functionality so that people have an easy template to follow, as issues like this one keep coming up.

@colegleason
Copy link
Author

I'm running into more issues here.

db.load's callbacks work fine, which means I can essentially 'block' until db has synced. The express middleware, on the other hand, does not expose access to the next() call in order to chain callbacks.

The issue here is that the below does not work:

exports.express = function(db, models) {                                                                                                                                                      
    load(db, function() {  // will be called once db.sync() completes                                                                                                                                                                  
        _.extend(models, db.models);                                                                                                                                                          
    });                                                                                                                                                                                       
};   

In unit testing with my test database, the request is executed quick enough that request.models is not set in the callback above. More accurately, it isn't set until after the test fails, but you know.

I'm not sure this extra middleware is worth it in the current state.

@grahamreeds
Copy link

I use

    app.use(orm.express("sqlite://sqlite3.s3db",{
        define: function(db, models){
            db.load('./models', function(err){
                models.person = db.models.Person;
                models.phone = db.models.Phone;
                // etc.
            });
        }
    }));

where my models/index.js looks like

module.exports = function(db,fn){
    db.load('person',   function(err){ if (err) { return fn(err); } });
    db.load('phone',    function(err){ if (err) { return fn(err); } });
    return fn();
};

Could be neater (especially the exports) but works for me.

@dxg
Copy link
Collaborator

dxg commented Aug 14, 2013

models/index.js has a bug; Since db.load is asynchronous, it means return fn() could be called first, and then once the async load finishes (with an error) it would call fn a second time.

That said, I'm not convinced that load needs to be async in the first place.

I've started work on the example app, but nothing to show just yet. Once it's done will have a discussion with everyone and hopefully update the api+docs.

@dresende
Copy link
Owner

db.load was created as async because you could have your model definition remotely or given by a service or something. The first part does a require of the given path but then the function called can take some time. The Express part should be changed to be async too.

dresende added a commit that referenced this issue Aug 20, 2013
`next` argument is optional and if set it will block until it's called
@dresende
Copy link
Owner

@colegleason please try this latest commit. If you define your function with a 3rd argument it will assume it's a next callback that you call when everything is loaded.

@SamuelBolduc
Copy link

Just a heads up, this should be updated in the readme as I had a pretty hard time figuring out how to declare it the correct way using the middleware (and not all in my app.js file)

@ptnplanet
Copy link

I just want to share my solution:

File stack.js

require defineModels = require('./model').define;
app.use(orm.express('sqlite://sqlite3.s3dbt', { define: defineModels }));

File model.js

var async = require('async'),
    definedModels;
module.exports.define = function (db, models, next) {
    definedModels = models;
    var curriedLoad = function (file) { return function (cb) { db.load(file, cb); }; };

    async.waterfall([
            curriedLoad('.model/user'),
            curriedLoad('.model/posting'),
            // ...
    ], function (err) {
        db.models.user.hasMany(db.models.posting);
        db.sync(next);
    });
};

// I use passport.js and need models without being able to access them through the request object.
module.exports.model = function (name) {
    return definedModels[name];
}

@dresende
Copy link
Owner

This will pass to the readme or the wiki as soon as we release a new version.

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

No branches or pull requests

6 participants