From 17588c5f8150c0c641d106dfb68e0ad4d4382dc4 Mon Sep 17 00:00:00 2001 From: Harminder Virk Date: Tue, 16 Aug 2016 22:50:44 +0530 Subject: [PATCH] perf(lucid:hooks): resolve hooks when adding resolving hooks on add will be a one time process to whereas earlier they were resolved on each action --- package.json | 1 + src/Lucid/Model/Mixins/Hooks.js | 34 +++++++----------------- src/Lucid/Model/index.js | 42 ++++++++++++++++++------------ test/unit/app/Model/Hooks/Users.js | 4 +++ test/unit/lucid.spec.js | 18 ++++++++----- 5 files changed, 52 insertions(+), 47 deletions(-) diff --git a/package.json b/package.json index 79990a5a..2b4f7adc 100644 --- a/package.json +++ b/package.json @@ -42,6 +42,7 @@ } }, "dependencies": { + "adonis-binding-resolver": "^1.0.1", "auto-loader": "git+https://github.com/thetutlage/node-auto-loader.git", "cat-log": "^1.0.2", "chance": "^1.0.3", diff --git a/src/Lucid/Model/Mixins/Hooks.js b/src/Lucid/Model/Mixins/Hooks.js index 64107ffc..19887999 100644 --- a/src/Lucid/Model/Mixins/Hooks.js +++ b/src/Lucid/Model/Mixins/Hooks.js @@ -11,7 +11,6 @@ const Hooks = exports = module.exports = {} const _ = require('lodash') -const Ioc = require('adonis-fold').Ioc /** * compose hooks for a given type by reading values @@ -25,18 +24,9 @@ const Ioc = require('adonis-fold').Ioc * @public */ Hooks.getHooks = function (type, handler) { - let beforeHooks = [] - let afterHooks = [] - const modelHooks = this.constructor.$modelHooks - if (!modelHooks) { - return [handler] - } - if (modelHooks[`before${type}`]) { - beforeHooks = _.map(modelHooks[`before${type}`], function (hook) { return hook.handler }) - } - if (modelHooks[`after${type}`]) { - afterHooks = _.map(modelHooks[`after${type}`], function (hook) { return hook.handler }) - } + const modelHooks = this.constructor.$modelHooks || {} + const beforeHooks = _.map(modelHooks[`before${type}`], 'handler') || [] + const afterHooks = _.map(modelHooks[`after${type}`], 'handler') || [] return beforeHooks.concat([handler]).concat(afterHooks) } @@ -53,21 +43,17 @@ Hooks.getHooks = function (type, handler) { * @public */ Hooks.composeHooks = function (scope, hooks) { - const Helpers = Ioc.use('Adonis/Src/Helpers') - const hookNameSpace = 'Model/Hooks' - function * noop () {} + return function * (next) { next = next || noop() - let i = hooks.length - while (i--) { - let hook = hooks[i] - if (typeof (hooks[i]) === 'string') { - const resolvedHook = Ioc.makeFunc(`${Helpers.makeNameSpace(hookNameSpace, hooks[i])}`) - hook = resolvedHook.instance[resolvedHook.method] + _.forEachRight(hooks, (hook) => { + if (typeof (hook) === 'function') { + next = hook.apply(scope, [next]) + } else if (hook.instance && hook.method) { + next = hook.instance[hook.method].apply(scope, [next]) } - next = hook.apply(scope, [next]) - } + }) yield * next } } diff --git a/src/Lucid/Model/index.js b/src/Lucid/Model/index.js index ccfdbf15..a31da3db 100644 --- a/src/Lucid/Model/index.js +++ b/src/Lucid/Model/index.js @@ -22,6 +22,11 @@ const proxyHandler = require('./proxyHandler') const Mixins = require('./Mixins') const Relations = require('../Relations') const BaseSerializer = require('../QueryBuilder/Serializers/Base') +const Ioc = require('adonis-fold').Ioc +const Resolver = require('adonis-binding-resolver') +const resolver = new Resolver(Ioc) + +const hookNameSpace = 'Model/Hooks' /** * returns a function that be executed with a key/value @@ -104,6 +109,22 @@ class Model { this.setJSON(values) } + /** + * validates whether hooks is of valid type + * or not + * + * @method _validateIsValidHookType + * + * @param {String} type + * + * @private + */ + static _validateIsValidHookType (type) { + if (validHookTypes.indexOf(type) <= -1) { + throw CE.InvalidArgumentException.invalidParameter(`${type} is not a valid hook type`) + } + } + /** * adds a new hook for a given type for a model. Note * this method has no way of checking duplicate @@ -123,30 +144,19 @@ class Model { * @public */ static addHook (type, name, handler) { - if (validHookTypes.indexOf(type) <= -1) { - throw CE.InvalidArgumentException.invalidParameter(`${type} is not a valid hook type`) - } + this._validateIsValidHookType(type) + const Helpers = Ioc.use('Adonis/Src/Helpers') - /** - * if handler is not defined, set name as handler. It is required coz - * we have 2nd parameter optional. - */ if (!handler) { handler = name name = null } - /** - * handler should be a reference to a string or a valid function. Strings are - * assumed to be a reference to hook namespace and if autoloading fails an - * error will be thrown when calling hook - */ - if (typeof (handler) !== 'function' && typeof (handler) !== 'string') { - throw CE.InvalidArgumentException.invalidParameter('hook handler must point to a valid generator method') - } + const resolvedHandler = typeof (handler) === 'string' ? Helpers.makeNameSpace(hookNameSpace, handler) : handler + resolver.validateBinding(resolvedHandler) this.$modelHooks[type] = this.$modelHooks[type] || [] - this.$modelHooks[type].push({handler, name}) + this.$modelHooks[type].push({handler: resolver.resolveBinding(resolvedHandler), name}) } /** diff --git a/test/unit/app/Model/Hooks/Users.js b/test/unit/app/Model/Hooks/Users.js index 7c19be5b..9fcd72a8 100644 --- a/test/unit/app/Model/Hooks/Users.js +++ b/test/unit/app/Model/Hooks/Users.js @@ -6,3 +6,7 @@ UsersHooks.validate = function * (next) { this.username = 'viahook' yield next } + +UsersHooks.log = function * (next) { + yield next +} diff --git a/test/unit/lucid.spec.js b/test/unit/lucid.spec.js index 58390438..4edda67b 100644 --- a/test/unit/lucid.spec.js +++ b/test/unit/lucid.spec.js @@ -1306,7 +1306,7 @@ describe('Lucid', function () { const fn = function () { User.addHook('beforeCreate') } - expect(fn).to.throw('InvalidArgumentException: E_INVALID_PARAMETER: hook handler must point to a valid generator method') + expect(fn).to.throw('InvalidArgumentException: E_INVALID_IOC_BINDING: Handler must point to a valid namespace or a closure') }) it('should add a hook for a given type', function () { @@ -1356,22 +1356,26 @@ describe('Lucid', function () { it('should be able to define multiple hooks in a go', function () { class User extends Model {} User.bootIfNotBooted() - User.defineHooks('beforeCreate', 'UsersHook.validate', 'UsersHook.log') + User.defineHooks('beforeCreate', 'Users.validate', 'Users.log') expect(User.$modelHooks['beforeCreate']).to.be.an('array') expect(User.$modelHooks['beforeCreate'].length).to.equal(2) - expect(User.$modelHooks['beforeCreate'][0].handler).to.equal('UsersHook.validate') - expect(User.$modelHooks['beforeCreate'][1].handler).to.equal('UsersHook.log') + expect(User.$modelHooks['beforeCreate'][0].handler).to.have.property('instance') + expect(User.$modelHooks['beforeCreate'][0].handler).to.have.property('method') + expect(User.$modelHooks['beforeCreate'][1].handler).to.have.property('instance') + expect(User.$modelHooks['beforeCreate'][1].handler).to.have.property('method') }) it('should override existing hooks when calling defineHooks', function () { class User extends Model {} User.bootIfNotBooted() User.addHook('beforeCreate', function * () {}) - User.defineHooks('beforeCreate', 'UsersHook.validate', 'UsersHook.log') + User.defineHooks('beforeCreate', 'Users.validate', 'Users.log') expect(User.$modelHooks['beforeCreate']).to.be.an('array') expect(User.$modelHooks['beforeCreate'].length).to.equal(2) - expect(User.$modelHooks['beforeCreate'][0].handler).to.equal('UsersHook.validate') - expect(User.$modelHooks['beforeCreate'][1].handler).to.equal('UsersHook.log') + expect(User.$modelHooks['beforeCreate'][0].handler).to.have.property('instance') + expect(User.$modelHooks['beforeCreate'][0].handler).to.have.property('method') + expect(User.$modelHooks['beforeCreate'][1].handler).to.have.property('instance') + expect(User.$modelHooks['beforeCreate'][1].handler).to.have.property('method') }) it('should execute beforeCreate hook when a model is saved to the database', function * () {