luin / express-promise

❤️ Middleware for easy rendering of async Query results.
MIT License
316 stars 19 forks source link

Sequelize objects have their methods/properties removed #9

Closed lingo closed 10 years ago

lingo commented 10 years ago

Sometimes I still need to use promises in my request handlers, and return Sequelize objects to my templates.

However, these sequelize objects are having any extra properties (instanceMethods etc) stripped by express-promise.

Example:

        app.get('/users', function(req, res, next) {
            req.account.getUsers()
                .then(function(users) {
                    // do something to users, or other processing
                    res.render('account/users', {
                        account: req.account,
                        users:   users
                    });
                }).catch(next);
        }
for user in users
  p= user.summary() // Custom instance method defined on model

In the above example we get an error, because express-promise is calling toJSON on my sequelize objects. See express-promise.js line 34, as below:

...
  if (typeof object.toJSON === 'function') {
    object = object.toJSON();
    if (!object || typeof object !== 'object') {
      return callback(null, object);
    }
  }
...

I'm not sure why toJSON needs calling here, but can it be removed for sequelize objects somehow?

luin commented 10 years ago

I am a Sequelize user too :) The only way to detect if an object is a promise is to check wether it has a then property, and as a result, the lib has to traverse all the properties recursively to make sure that we will never leaving a promise unresolved.

toJSON is needed here to improve the performance. For example, a typical Sequelize model instance contains many properties, most of which are Sequelize's utility functions(update, remove...), which has nothing to do with our data but will take much time if we traverse them.

I know that sometimes we still need to call custom instance methods in the template. So I create another repo express-sequelize: https://github.com/luin/express-sequelize. This module is depend on Sequelize, so it's able to recognise a Sequelize instance. When traversing an object recursively, if the object is a Sequelize instance, we can stop traversing it. In this way, toJSON is not needed anymore but at the same time we still have a good performance. However, express-sequelize is designed to used with Express 3.0 and I'm afraid it isn't compatible with Express 4.0.

I'm planning to add an option to the express-promise in order to avoid calling toJSON without affect the performance, which is using a custom function to indicate whether a object should be traversed recursively.

lingo commented 10 years ago

Thank you for the rapid and informative response!

I'll look at express-sequelize and see how I get on. Do you know right now what makes it incompatible? If I have some time I would contribute some pull-requests.

Thanks again

luin commented 10 years ago

Express-sequelize is deprecated and I suggest you to use express-promise :-)

I add an option to avoid calling toJSON: https://github.com/luin/express-promise/blob/master/README.md#skip-traverse

The test case is here: https://github.com/luin/express-promise/blob/master/test/skip_rule.js

Could you please help me testing whether it works? Thank you!

lingo commented 10 years ago

Wow, that's awesome!

I'll try that out later today and update here with feedback. Thanks!

lingo commented 10 years ago

Excellent, it works fine for me. The unit tests all pass perfectly, and in my code I'm using the following to detect sequelize instances, and it seems to work well.

    app.use(require('express-promise')({
        skipTraverse: function(object) {
            // SEE: https://github.com/luin/express-promise/issues/9
            return typeof object.sequelize !== 'undefined';
        }
    }));

Thank you!!

luin commented 10 years ago

Glad to hear that! Thank you for the feedback :-)