withHooks + custom methods (feathersjs#924)

* open hooks workflow to custom methods

* remove console.log

* remove useless line

* new service 'methods' shape

* pick arguments in validateHook for custom methods

* separate validateHook and pickArgsHook

* extend pickArgsHook to all methods

* remove variable with single usage

* validate before pick arguments

* export withHooks method

* add tests for withHooks

* fix getHookArray

* replace Object.assign by assignation

* simplify names

* add activeHooks method

* all methods in service.methods

* rename activeHooks to activateHooks

* remove useless assignation

* use Object.getOwnPropertyNames instead of Object.keys

* update @featherjs/commons

* fix test for node 6

* fix withHooks

* Update hooks.js

* use Object.defineProperty in the activateHooks method

.gitignore

@@ -28,3 +28,6 @@ node_modules
 
 # Users Environment Variables
 .lock-wscript
+
+# IDEs
+.idea

lib/hooks.js

@@ -8,119 +8,184 @@ const {
   makeArguments
 } = hooks;
 
+const ACTIVATE_HOOKS = typeof Symbol !== 'undefined'
+  ? Symbol('__feathersActivateHooks')
+  : '__feathersActivateHooks';
+
+function getHookArray (hooks, type) {
+  return hooks && hooks[type] && Array.isArray(hooks[type])
+    ? hooks[type]
+    : hooks && hooks[type]
+      ? [hooks[type]]
+      : [];
+}
+
+const withHooks = function withHooks ({
+  app,
+  service,
+  method
+}) {
+  return (hooks = {}) => (...args) => {
+    const returnHook = args[args.length - 1] === true
+      ? args.pop() : false;
+
+    // A reference to the original method
+    const _super = service._super ? service._super.bind(service) : service[method].bind(service);
+    // Create the hook object that gets passed through
+    const hookObject = createHookObject(method, {
+      type: 'before', // initial hook object type
+      service,
+      app
+    });
+
+    // A hook that pick arguments for methods defined in `service.methods`
+    const pickArgsHook = context => {
+      const argsObject = args.reduce(
+        (result, value, index) => {
+          result[service.methods[method][index]] = value;
+          return result;
+        },
+        { params: {} }
+      );
+
+      Object.assign(context, argsObject);
+
+      return context;
+    };
+
+    // Process all before hooks
+    return processHooks.call(service, [pickArgsHook, ...getHookArray(hooks, 'before')], hookObject)
+    // Use the hook object to call the original method
+      .then(hookObject => {
+        // If `hookObject.result` is set, skip the original method
+        if (typeof hookObject.result !== 'undefined') {
+          return hookObject;
+        }
+
+        // Otherwise, call it with arguments created from the hook object
+        const promise = _super(...makeArguments(hookObject));
+
+        if (!isPromise(promise)) {
+          throw new Error(`Service method '${hookObject.method}' for '${hookObject.path}' service must return a promise`);
+        }
+
+        return promise.then(result => {
+          hookObject.result = result;
+
+          return hookObject;
+        });
+      })
+      // Make a (shallow) copy of hookObject from `before` hooks and update type
+      .then(hookObject => Object.assign({}, hookObject, { type: 'after' }))
+      // Run through all `after` hooks
+      .then(hookObject => {
+        // Combine all app and service `after` and `finally` hooks and process
+        const hookChain = getHookArray(hooks, 'after')
+          .concat(getHookArray(hooks, 'finally'));
+
+        return processHooks.call(service, hookChain, hookObject);
+      })
+      .then(hookObject =>
+        // Finally, return the result
+        // Or the hook object if the `returnHook` flag is set
+        returnHook ? hookObject : hookObject.result
+      )
+      // Handle errors
+      .catch(error => {
+        // Combine all app and service `error` and `finally` hooks and process
+        const hookChain = getHookArray(hooks, 'error')
+          .concat(getHookArray(hooks, 'finally'));
+
+        // A shallow copy of the hook object
+        const errorHookObject = _.omit(Object.assign({}, error.hook, hookObject, {
+          type: 'error',
+          original: error.hook,
+          error
+        }), 'result');
+
+        return processHooks.call(service, hookChain, errorHookObject)
+          .catch(error => {
+            errorHookObject.error = error;
+
+            return errorHookObject;
+          })
+          .then(hook => {
+            if (returnHook) {
+              // Either resolve or reject with the hook object
+              return typeof hook.result !== 'undefined' ? hook : Promise.reject(hook);
+            }
+
+            // Otherwise return either the result if set (to swallow errors)
+            // Or reject with the hook error
+            return typeof hook.result !== 'undefined' ? hook.result : Promise.reject(hook.error);
+          });
+      });
+  };
+};
+
 // A service mixin that adds `service.hooks()` method and functionality
 const hookMixin = exports.hookMixin = function hookMixin (service) {
   if (typeof service.hooks === 'function') {
     return;
   }
 
+  service.methods = Object.getOwnPropertyNames(service)
+    .filter(key => typeof service[key] === 'function' && service[key][ACTIVATE_HOOKS])
+    .reduce((result, methodName) => {
+      result[methodName] = service[methodName][ACTIVATE_HOOKS];
+      return result;
+    }, service.methods || {});
+
+  Object.assign(service.methods, {
+    find: ['params'],
+    get: ['id', 'params'],
+    create: ['data', 'params'],
+    update: ['id', 'data', 'params'],
+    patch: ['id', 'data', 'params'],
+    remove: ['id', 'params']
+  });
+
   const app = this;
-  const methods = app.methods;
+  const methodNames = Object.keys(service.methods);
   const mixin = {};
 
   // Add .hooks method and properties to the service
-  enableHooks(service, methods, app.hookTypes);
+  enableHooks(service, methodNames, app.hookTypes);
 
   // Assemble the mixin object that contains all "hooked" service methods
-  methods.forEach(method => {
+  methodNames.forEach(method => {
     if (typeof service[method] !== 'function') {
       return;
     }
 
     mixin[method] = function () {
       const service = this;
       const args = Array.from(arguments);
-      // If the last argument is `true` we want to return
-      // the actual hook object instead of the result
-      const returnHook = args[args.length - 1] === true
-        ? args.pop() : false;
-
-      // A reference to the original method
-      const _super = service._super.bind(service);
-      // Create the hook object that gets passed through
-      const hookObject = createHookObject(method, args, {
-        type: 'before', // initial hook object type
-        service,
-        app
-      });
+      const returnHook = args[args.length - 1] === true;
+
       // A hook that validates the arguments and will always be the very first
       const validateHook = context => {
-        validateArguments(method, args);
+        validateArguments(service.methods, method, returnHook ? args.slice(0, -1) : args);
 
         return context;
       };
-      // The `before` hook chain (including the validation hook)
-      const beforeHooks = [ validateHook, ...getHooks(app, service, 'before', method) ];
-
-      // Process all before hooks
-      return processHooks.call(service, beforeHooks, hookObject)
-        // Use the hook object to call the original method
-        .then(hookObject => {
-          // If `hookObject.result` is set, skip the original method
-          if (typeof hookObject.result !== 'undefined') {
-            return hookObject;
-          }
-
-          // Otherwise, call it with arguments created from the hook object
-          const promise = _super(...makeArguments(hookObject));
-
-          if (!isPromise(promise)) {
-            throw new Error(`Service method '${hookObject.method}' for '${hookObject.path}' service must return a promise`);
-          }
 
-          return promise.then(result => {
-            hookObject.result = result;
+      // Needed
+      service._super = service._super.bind(service);
 
-            return hookObject;
-          });
-        })
-        // Make a (shallow) copy of hookObject from `before` hooks and update type
-        .then(hookObject => Object.assign({}, hookObject, { type: 'after' }))
-        // Run through all `after` hooks
-        .then(hookObject => {
-          // Combine all app and service `after` and `finally` hooks and process
-          const afterHooks = getHooks(app, service, 'after', method, true);
-          const finallyHooks = getHooks(app, service, 'finally', method, true);
-          const hookChain = afterHooks.concat(finallyHooks);
-
-          return processHooks.call(service, hookChain, hookObject);
-        })
-        .then(hookObject =>
-          // Finally, return the result
-          // Or the hook object if the `returnHook` flag is set
-          returnHook ? hookObject : hookObject.result
-        )
-        // Handle errors
-        .catch(error => {
-          // Combine all app and service `error` and `finally` hooks and process
-          const errorHooks = getHooks(app, service, 'error', method, true);
-          const finallyHooks = getHooks(app, service, 'finally', method, true);
-          const hookChain = errorHooks.concat(finallyHooks);
-
-          // A shallow copy of the hook object
-          const errorHookObject = _.omit(Object.assign({}, error.hook, hookObject, {
-            type: 'error',
-            original: error.hook,
-            error
-          }), 'result');
-
-          return processHooks.call(service, hookChain, errorHookObject)
-            .catch(error => {
-              errorHookObject.error = error;
-
-              return errorHookObject;
-            })
-            .then(hook => {
-              if (returnHook) {
-                // Either resolve or reject with the hook object
-                return typeof hook.result !== 'undefined' ? hook : Promise.reject(hook);
-              }
-
-              // Otherwise return either the result if set (to swallow errors)
-              // Or reject with the hook error
-              return typeof hook.result !== 'undefined' ? hook.result : Promise.reject(hook.error);
-            });
-        });
+      return withHooks({
+        app,
+        service,
+        method
+      })({
+        before: [
+          validateHook,
+          ...getHooks(app, service, 'before', method)
+        ],
+        after: getHooks(app, service, 'after', method, true),
+        error: getHooks(app, service, 'error', method, true),
+        finally: getHooks(app, service, 'finally', method, true)
+      })(...args);
     };
   });
 
@@ -141,3 +206,14 @@ module.exports = function () {
     app.mixins.push(hookMixin);
   };
 };
+
+module.exports.withHooks = withHooks;
+
+module.exports.ACTIVATE_HOOKS = ACTIVATE_HOOKS;
+
+module.exports.activateHooks = function activateHooks (args) {
+  return fn => {
+    Object.defineProperty(fn, ACTIVATE_HOOKS, { value: args });
+    return fn;
+  };
+};

lib/index.js

@@ -2,6 +2,7 @@ const { hooks } = require('@feathersjs/commons');
 const Proto = require('uberproto');
 const Application = require('./application');
 const version = require('./version');
+const { ACTIVATE_HOOKS, activateHooks } = require('./hooks');
 
 function createApplication () {
   const app = {};
@@ -16,6 +17,8 @@ function createApplication () {
 
 createApplication.version = version;
 createApplication.SKIP = hooks.SKIP;
+createApplication.ACTIVATE_HOOKS = ACTIVATE_HOOKS;
+createApplication.activateHooks = activateHooks;
 
 module.exports = createApplication;
 

package.json

@@ -50,7 +50,7 @@
     "node": ">= 6"
   },
   "dependencies": {
-    "@feathersjs/commons": "^1.4.1",
+    "@feathersjs/commons": "^2.0.0",
     "debug": "^3.1.0",
     "events": "^3.0.0",
     "uberproto": "^2.0.2"

test/hooks/hooks.test.js

@@ -242,4 +242,66 @@ describe('hooks basics', () => {
       });
     });
   });
+
+  it('can register hooks on a custom method', () => {
+    const app = feathers().use('/dummy', {
+      methods: {
+        custom: ['id', 'data', 'params']
+      },
+      get () {},
+      custom (id, data, params) {
+        return Promise.resolve([id, data, params]);
+      },
+      // activateHooks is usable as a decorator: @activateHooks(['id', 'data', 'params'])
+      other: feathers.activateHooks(['id', 'data', 'params'])(
+        (id, data, params) => {
+          return Promise.resolve([id, data, params]);
+        }
+      )
+    });
+
+    app.service('dummy').hooks({
+      before: {
+        all (context) {
+          context.test = ['all::before'];
+        },
+        custom (context) {
+          context.test.push('custom::before');
+        }
+      },
+      after: {
+        all (context) {
+          context.test.push('all::after');
+        },
+        custom (context) {
+          context.test.push('custom::after');
+        }
+      }
+    });
+
+    const args = [1, { test: 'ok' }, { provider: 'rest' }];
+
+    assert.deepEqual(app.service('dummy').methods, {
+      find: ['params'],
+      get: ['id', 'params'],
+      create: ['data', 'params'],
+      update: ['id', 'data', 'params'],
+      patch: ['id', 'data', 'params'],
+      remove: ['id', 'params'],
+      custom: ['id', 'data', 'params'],
+      other: ['id', 'data', 'params']
+    });
+
+    return app.service('dummy').custom(...args, true)
+      .then(hook => {
+        assert.deepEqual(hook.result, args);
+        assert.deepEqual(hook.test, ['all::before', 'custom::before', 'all::after', 'custom::after']);
+
+        app.service('dummy').other(...args, true)
+          .then(hook => {
+            assert.deepEqual(hook.result, args);
+            assert.deepEqual(hook.test, ['all::before', 'all::after']);
+          });
+      });
+  });
 });