Updating hooks API to use hook objects (#1)

Author: daffl

README.md

@@ -71,52 +71,79 @@ var todoService = app.lookup('todos');
 
 ### `service.before(beforeHooks)`
 
-`before` hooks allow to pre-process service call parameters. They will be called with the original service parameters
-and a callback which should be called with the same (potentially modified) parameters and an error (for example, when
-a user is not authorized, `null` for no error).
+`before` hooks allow to pre-process service call parameters. They will be called with the hook object
+and a callback which should be called with any errors or no arguments or `null` and the modified hook object.
+The hook object contains information about the intercepted method and for `before` hooks can have the following properties:
+
+- __method__ - The method name
+- __type__ - The hook type (`before` or `after`)
+- __callback__ - The original callback (can be replaced but shouldn't be called in your hook)
+- __params__ - The service method parameters
+- __data__ - The request data (for `create`, `update` and `patch`)
+- __id__ - The id (for `get`, `remove`, `update` and `patch`)
+
+All properties of the hook object can be modified and the modified data will be used for the actual service method
+call. This is very helpful for pre-processing parameters and massaging data when creating or updating.
+
 The following example checks if a user has been passed to the services `find` method and returns an error if not
 and also adds a `createdAt` property to a newly created todo:
 
 ```js
 todoService.before({
-  find: function (params, callback) {
-    if (!params.user) {
+  find: function (hook, next) {
+    if (!hook.params.user) {
       return callback(new Error('You are not logged in'));
     }
-    callback(null, params);
+    
+    next();
   },
 
-  create: function(data, params, callback) {
-    data.createdAt = new Date();
+  create: function(hook, next) {
+    hook.data.createdAt = new Date();
 
-    callback(null, data, params);
+    next();
+    // Or
+    next(null, hook);
   }
 });
 ```
 
 ### `service.after(afterHooks)`
 
-`after` hooks will be called with the result of the service call, the original parameters and a callback. The following example
-filters the data returned by a `find` service call based on a users company id and checks if the current user is allowed
-to retrieve the data returned by `get` (that is, they have the same company id):
+`after` hooks will be called with a similar hook object than `before` hooks but additionally contain a `result`
+property with the service call results:
+
+- __method__ - The method name
+- __type__ - The hook type (`before` or `after`)
+- __result__ - The service call result data
+- __callback__ - The original callback (can be replaced but shouldn't be called in your hook)
+- __params__ - The service method parameters
+- __data__ - The request data (for `create`, `update` and `patch`)
+- __id__ - The id (for `get`, `remove`, `update` and `patch`)
+
+In any `after` hook, only modifications to the `result` object will have any effect. This is a good place to filter or
+post-process the data retrieved by a service and also add some additional authorization that needs the actual data.
+
+The following example filters the data returned by a `find` service call based on a users company id
+and checks if the current user is allowed to retrieve the data returned by `get` (that is, they have the same company id):
 
 ```js
 todoService.after({
-  find: function (data, params, callback) {
+  find: function (hook, next) {
     // Manually filter the find results
-    var filtered = _.filter(data, function (current) {
+    hook.result = _.filter(hook.result, function (current) {
       return current.companyId === params.user.companyId;
     });
 
-    callback(null, filtered);
+    next();
   },
 
-  get: function (data, id, params, callback) {
-    if (data.companyId !== params.user.companyId) {
+  get: function (hook, next) {
+    if (hook.result.companyId !== hook.params.user.companyId) {
       return callback(new Error('You are not authorized to access this information'));
     }
 
-    callback(null, data);
+    next();
   }
 });
 ```
@@ -129,37 +156,39 @@ You can also add `before` and `after` hooks to your initial service object right
 ```js
 var TodoService = {
   before: {
-    find: function (params, callback) {
-      if (!params.user) {
+    find: function (hook, next) {
+      if (!hook.params.user) {
         return callback(new Error('You are not logged in'));
       }
-      callback(null, params);
+      
+      next();
     },
-
-    get: function (id, params, callback) {
-      // Pre-process the params passed to the actual service
-      params.something = 'test';
-
-      callback(null, id, params);
+  
+    create: function(hook, next) {
+      hook.data.createdAt = new Date();
+  
+      next();
+      // Or
+      next(null, hook);
     }
   },
 
   after: {
-    find: function (data, params, callback) {
+    find: function (hook, next) {
       // Manually filter the find results
-      var filtered = _.filter(data, function (current) {
+      hook.result = _.filter(hook.result, function (current) {
         return current.companyId === params.user.companyId;
       });
-
-      callback(null, filtered);
+  
+      next();
     },
-
-    get: function (data, id, params, callback) {
-      if (data.companyId !== params.user.companyId) {
+  
+    get: function (hook, next) {
+      if (hook.result.companyId !== hook.params.user.companyId) {
         return callback(new Error('You are not authorized to access this information'));
       }
-
-      callback(null, data);
+  
+      next();
     }
   }
 }

lib/after.js

@@ -1,4 +1,5 @@
 var _ = require('lodash');
+var utils = require('./utils');
 
 module.exports = function (service) {
   if (typeof service.mixin !== 'function') {
@@ -19,22 +20,41 @@ module.exports = function (service) {
         result[name] = function () {
           var self = this;
           var args = _.toArray(arguments);
-          var oldCallback = args.pop();
-          var newCallback = function (error, data) {
+					var hookObject = utils.hookObject(name, args);
+					// The callback for the after hook
+					var hookCallback = function(error, newHookObject) {
+						// Call the callback with the result we set in `newCallback`
+						var hook = newHookObject || hookObject;
+
+						if (error) {
+							// Call the old callback with the hook error
+							return hook.callback(error);
+						}
+
+						hook.callback(null, hook.result);
+					};
+					// The new _super method callback
+          var newCallback = function (error, result) {
             if (error) {
-              return oldCallback(error);
+							// Call the old callback with the error
+              return hookObject.callback(error);
             }
 
-            var hookArgs = args;
-
-            // Add data and the oldCallback before running the hook
-            hookArgs.unshift(data);
-            hookArgs.push(oldCallback);
+						// Set hookObject result
+						hookObject.result = result;
 
-            hook.apply(self, hookArgs);
+						// Call the hook object
+						hook.call(self, hookObject, hookCallback);
           };
 
-          return this._super.apply(this, args.concat(newCallback));
+					hookObject.type = 'after';
+
+					// Remove the old callback and replace with the new callback that runs the hook
+					args.pop();
+					args.push(newCallback);
+
+					// Call super method
+          return this._super.apply(this, args);
         };
       });
 

lib/before.js

@@ -1,4 +1,5 @@
 var _ = require('lodash');
+var utils = require('./utils');
 
 module.exports = function (service) {
   if (typeof service.mixin !== 'function') {
@@ -20,28 +21,22 @@ module.exports = function (service) {
           var self = this;
           // The original method
           var _super = this._super;
-          var args = _.toArray(arguments);
-          // Store the original callback
-          var oldCallback = args.pop();
-
-          // The callback that is being called by the .before hook
-          var newCallback = function () {
-            var error = arguments[0];
-
-            if (error) {
-              // We got an error
-              return oldCallback(error);
-            }
-
-            // Use either the original arguments or the ones we got from the callback
-            var newArguments = arguments.length > 1 ?
-              Array.prototype.slice.call(arguments, 1, arguments.length) : args;
-
-            // Call the original method with the old callback added
-            _super.apply(self, newArguments.concat(oldCallback));
-          };
-
-          hook.apply(self, args.concat(newCallback));
+					// The hook data object
+          var hookObject = utils.hookObject(name, arguments);
+					// The callback for the hook
+					var hookCallback = function(error, newHookObject) {
+						if (error) {
+							// We got an error
+							return hookObject.callback(error);
+						}
+
+						// Call the _super method. We either use the original hook object
+						// to create the arguments or the new one passed to the hook callback
+						_super.apply(self, utils.makeArguments(newHookObject || hookObject));
+					};
+
+					hookObject.type = 'before';
+          hook.call(self, hookObject, hookCallback);
         };
       });
 

lib/utils.js

@@ -0,0 +1,59 @@
+function getOrRemove(args) {
+	return {
+		id: args[0],
+		params: args[1],
+		callback: args[2]
+	};
+}
+
+function updateOrPatch(args) {
+	return {
+		id: args[0],
+		data: args[1],
+		params: args[2],
+		callback: args[3]
+	};
+}
+
+exports.converters = {
+	find: function(args) {
+		return {
+			params: args[0],
+			callback: args[1]
+		};
+	},
+	create: function(args) {
+		return {
+			data: args[0],
+			params: args[1],
+			callback: args[2]
+		};
+	},
+	get: getOrRemove,
+	remove: getOrRemove,
+	update: updateOrPatch,
+	patch: updateOrPatch
+};
+
+exports.hookObject = function(method, args) {
+	var hook = exports.converters[method](args);
+	hook.method = method;
+
+	return hook;
+};
+
+exports.makeArguments = function(hookObject) {
+	var result = [];
+	if(hookObject.id) {
+		result.push(hookObject.id);
+	}
+
+	if(hookObject.data) {
+		result.push(hookObject.data);
+	}
+
+	result.push(hookObject.params || {});
+	result.push(hookObject.callback);
+
+	return result;
+};

test/after.test.js

@@ -1,4 +1,3 @@
-var _ = require('lodash');
 var assert = require('assert');
 var feathers = require('feathers');
 
@@ -8,8 +7,12 @@ describe('.after hooks', function () {
   it('gets mixed into a service and modifies data', function (done) {
     var dummyService = {
       after: {
-        create: function (result, data, params, callback) {
-          callback(null, _.extend({ some: 'thing' }, result));
+        create: function (hook, next) {
+					assert.equal(hook.type, 'after');
+
+					hook.result.some = 'thing';
+
+					next(null, hook);
         }
       },
 
@@ -30,8 +33,8 @@ describe('.after hooks', function () {
   it('returns errors', function (done) {
     var dummyService = {
       after: {
-        update: function (result, id, data, params, callback) {
-          callback(new Error('This did not work'));
+        update: function (hook, next) {
+          next(new Error('This did not work'));
         }
       },
 
@@ -84,14 +87,17 @@ describe('.after hooks', function () {
     var service = app.lookup('dummy');
 
     service.after({
-      create: function (result, data, params, callback) {
-        callback(null, _.extend({ some: 'thing' }, result));
+      create: function (hook, next) {
+				hook.result.some = 'thing';
+        next();
       }
     });
 
     service.after({
-      create: function (result, data, params, callback) {
-        callback(null, _.extend({ other: 'stuff' }, result));
+      create: function (hook, next) {
+				hook.result.other = 'stuff';
+
+				next();
       }
     });