fix: Clean up hooks code (#1407)

packages/express/test/rest.test.js

@@ -249,7 +249,8 @@ describe('@feathersjs/express/rest provider', () => {
                 arguments: ['dishes', paramsWithHeaders ],
                 type: 'error',
                 method: 'get',
-                path: 'hook-error'
+                path: 'hook-error',
+                original: data.hook.original
               },
               error: { message: 'I blew up' }
             });

packages/feathers/lib/hooks/base.js

@@ -3,16 +3,14 @@ const { _ } = require('@feathersjs/commons');
 const assignArguments = context => {
   const { service, method } = context;
   const parameters = service.methods[method];
-  const argsObject = context.arguments.reduce((result, value, index) => {
-    result[parameters[index]] = value;
-    return result;
-  }, {});
 
-  if (!argsObject.params) {
-    argsObject.params = {};
-  }
+  context.arguments.forEach((value, index) => {
+    context[parameters[index]] = value;
+  });
 
-  Object.assign(context, argsObject);
+  if (!context.params) {
+    context.params = {};
+  }
 
   return context;
 };

packages/feathers/lib/hooks/index.js

@@ -1,4 +1,4 @@
-const { hooks, isPromise, _ } = require('@feathersjs/commons');
+const { hooks, isPromise } = require('@feathersjs/commons');
 const baseHooks = require('./base');
 
 const {
@@ -9,11 +9,6 @@ const {
   ACTIVATE_HOOKS
 } = hooks;
 
-const makeArguments = (service, method, hookObject) => service.methods[ method ].reduce((result, value) => ([
-  ...result,
-  hookObject[ value ]
-]), []);
-
 const withHooks = function withHooks ({
   app,
   service,
@@ -33,8 +28,6 @@ const withHooks = function withHooks ({
       const returnHook = args[args.length - 1] === true
         ? args.pop() : false;
 
-      // A reference to the original method
-      const _super = original || service[method].bind(service);
       // Create the hook object that gets passed through
       const hookObject = createHookObject(method, {
         type: 'before', // initial hook object type
@@ -43,70 +36,93 @@ const withHooks = function withHooks ({
         app
       });
 
-      // Process all before hooks
-      return processHooks.call(service, baseHooks.concat(hooks.before), hookObject)
-        // Use the hook object to call the original method
+      return Promise.resolve(hookObject)
+
+        // Run `before` hooks
+        .then(hookObject => {
+          return processHooks.call(service, baseHooks.concat(hooks.before), hookObject);
+        })
+
+        // Run 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(service, method, hookObject));
-
-          if (!isPromise(promise)) {
-            throw new Error(`Service method '${hookObject.method}' for '${hookObject.path}' service must return a promise`);
-          }
+          const promise = new Promise(resolve => {
+            const func = original || service[method];
+            const args = service.methods[method].map((value) => hookObject[value]);
+            const result = func.apply(service, args);
 
-          return promise.then(result => {
-            hookObject.result = result;
+            if (!isPromise(result)) {
+              throw new Error(`Service method '${hookObject.method}' for '${hookObject.path}' service must return a promise`);
+            }
 
-            return hookObject;
+            resolve(result);
           });
+
+          return promise
+            .then(result => {
+              hookObject.result = result;
+              return hookObject;
+            })
+            .catch(error => {
+              error.hook = hookObject;
+              throw error;
+            });
         })
-        // Make a (shallow) copy of hookObject from `before` hooks and update type
-        .then(hookObject => Object.assign({}, hookObject, { type: 'after' }))
-        // Run through all `after` hooks
+
+        // Run `after` hooks
         .then(hookObject => {
-          // Combine all app and service `after` and `finally` hooks and process
-          const hookChain = hooks.after.concat(hooks.finally);
+          const afterHookObject = Object.assign({}, hookObject, {
+            type: 'after'
+          });
 
-          return processHooks.call(service, hookChain, hookObject);
+          return processHooks.call(service, hooks.after, afterHookObject);
         })
-        .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 = hooks.error.concat(hooks.finally);
 
-          // A shallow copy of the hook object
-          const errorHookObject = _.omit(Object.assign({}, error.hook, hookObject, {
+        // Run `errors` hooks
+        .catch(error => {
+          const errorHookObject = Object.assign({}, error.hook, {
             type: 'error',
             original: error.hook,
-            error
-          }), 'result');
+            error,
+            result: undefined
+          });
 
-          return processHooks.call(service, hookChain, errorHookObject)
+          return processHooks.call(service, hooks.error, errorHookObject)
             .catch(error => {
-              errorHookObject.error = error;
+              const errorHookObject = Object.assign({}, error.hook, {
+                error,
+                result: undefined
+              });
+
+              return errorHookObject;
+            });
+        })
+
+        // Run `finally` hooks
+        .then(hookObject => {
+          return processHooks.call(service, hooks.finally, hookObject)
+            .catch(error => {
+              const errorHookObject = Object.assign({}, error.hook, {
+                error,
+                result: undefined
+              });
 
               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);
             });
+        })
+
+        // Resolve with a result or reject with an error
+        .then(hookObject => {
+          if (typeof hookObject.error !== 'undefined' && typeof hookObject.result === 'undefined') {
+            return Promise.reject(returnHook ? hookObject : hookObject.error);
+          } else {
+            return returnHook ? hookObject : hookObject.result;
+          }
         });
     };
   };

packages/feathers/test/hooks/error.test.js

@@ -11,7 +11,7 @@ describe('`error` hooks', () => {
     });
     const service = app.service('dummy');
 
-    afterEach(() => service.__hooks.error.get.pop());
+    afterEach(() => service.__hooks.error.get = []);
 
     it('basic error hook', () => {
       service.hooks({
@@ -154,6 +154,30 @@ describe('`error` hooks', () => {
       return app.service('dummy').get(1)
         .then(result => assert.strictEqual(result, null));
     });
+
+    it('uses the current hook object if thrown in a service method', () => {
+      const app = feathers().use('/dummy', {
+        get () {
+          return Promise.reject(new Error('Something went wrong'));
+        }
+      });
+
+      const service = app.service('dummy');
+
+      service.hooks({
+        before (hook) {
+          return { ...hook, id: 42 };
+        },
+        error (hook) {
+          assert.strictEqual(hook.id, 42);
+        }
+      });
+
+      return service.get(1).then(
+        () => assert.fail('Should never get here'),
+        error => assert.strictEqual(error.message, 'Something went wrong')
+      );
+    });
   });
 
   describe('error in hooks', () => {

packages/feathers/test/hooks/finally.test.js

@@ -73,4 +73,36 @@ describe('`finally` hooks', () => {
       }
     );
   });
+
+  it('runs once, sets error if throws', () => {
+    const app = feathers().use('/dummy', {
+      get (id) {
+        return Promise.resolve({ id });
+      }
+    });
+
+    const service = app.service('dummy');
+
+    let count = 0;
+
+    service.hooks({
+      error (hook) {
+        assert.fail('Should never get here (error hook)');
+      },
+      finally: [
+        function (hook) {
+          assert.strictEqual(++count, 1, 'This should be called only once');
+          throw new Error('This did not work');
+        },
+        function (hook) {
+          assert.fail('Should never get here (second finally hook)');
+        }
+      ]
+    });
+
+    return service.get(42).then(
+      () => assert.fail('Should never get here (result resolve)'),
+      error => assert.strictEqual(error.message, 'This did not work')
+    );
+  });
 });

packages/transport-commons/src/channels/mixins.ts

@@ -11,7 +11,7 @@ const ALL_EVENTS = Symbol('@feathersjs/transport-commons/all-events');
 export const keys = {
   PUBLISHERS: PUBLISHERS as typeof PUBLISHERS,
   CHANNELS: CHANNELS as typeof CHANNELS,
-  ALL_EVENTS: ALL_EVENTS as typeof ALL_EVENTS,
+  ALL_EVENTS: ALL_EVENTS as typeof ALL_EVENTS
 };
 
 export interface ChannelMixin {