feat(core): Improve legacy hooks integration

packages/express/test/rest.test.ts

@@ -244,8 +244,7 @@ describe('@feathersjs/express/rest provider', () => {
               type: null,
               event: null,
               method: 'get',
-              path: 'hook-error',
-              original: data.hook.original
+              path: 'hook-error'
             },
             error: { message: 'I blew up' }
           });

packages/feathers/src/hooks/index.ts

@@ -7,13 +7,17 @@ import {
 import { defaultServiceArguments, getHookMethods } from '../service';
 import {
   collectLegacyHooks,
-  enableLegacyHooks,
-  fromAfterHook,
-  fromBeforeHook,
-  fromErrorHooks
+  enableLegacyHooks
 } from './legacy';
 
-export { fromAfterHook, fromBeforeHook, fromErrorHooks };
+export {
+  fromBeforeHook,
+  fromBeforeHooks,
+  fromAfterHook,
+  fromAfterHooks,
+  fromErrorHook,
+  fromErrorHooks
+} from './legacy';
 
 export function createContext (service: Service, method: string, data: HookContextData = {}) {
   const createContext = (service as any)[method].createContext;

packages/feathers/src/hooks/legacy.ts

@@ -1,61 +1,40 @@
-import { _ } from '../dependencies';
-import { LegacyHookFunction } from '../declarations';
+import { HookFunction, LegacyHookFunction, LegacyHookMap } from '../declarations';
+import { defaultServiceMethods } from '../service';
 
-const { each } = _;
-const mergeContext = (context: any) => (res: any) => {
-  if (res && res !== context) {
-    Object.assign(context, res);
-  }
-  return res;
-}
+const runHook = <A, S> (hook: LegacyHookFunction<A, S>, context: any, type?: string) => {
+  if (type) context.type = type;
+  return Promise.resolve(hook.call(context.self, context))
+    .then((res: any) => {
+      if (type) context.type = null;
+      if (res && res !== context) {
+        Object.assign(context, res);
+      }
+    });
+};
 
-export function fromBeforeHook (hook: LegacyHookFunction) {
-  return (context: any, next: any) => {
-    context.type = 'before';
-
-    return Promise.resolve(hook.call(context.self, context))
-      .then(mergeContext(context))
-      .then(() => {
-        context.type = null;
-        return next();
-      });
+export function fromBeforeHook<A, S> (hook: LegacyHookFunction<A, S>): HookFunction<A, S> {
+  return (context, next) => {
+    return runHook(hook, context, 'before').then(next);
   };
 }
 
-export function fromAfterHook (hook: LegacyHookFunction) {
-  return (context: any, next: any) => {
-    return next()
-      .then(() => {
-        context.type = 'after';
-        return hook.call(context.self, context)
-      })
-      .then(mergeContext(context))
-      .then(() => {
-        context.type = null;
-      });
+export function fromAfterHook<A, S> (hook: LegacyHookFunction<A, S>): HookFunction<A, S> {
+  return (context, next) => {
+    return next().then(() => runHook(hook, context, 'after'));
   }
 }
 
-export function fromErrorHooks (hooks: LegacyHookFunction[]) {
-  return (context: any, next: any) => {
+export function fromErrorHook<A, S> (hook: LegacyHookFunction<A, S>): HookFunction<A, S> {
+  return (context, next) => {
     return next().catch((error: any) => {
-      let promise: Promise<any> = Promise.resolve();
-
-      context.original = { ...context };
-      context.error = error;
-      context.type = 'error';
-
-      delete context.result;
-
-      for (const hook of hooks) {
-        promise = promise.then(() => hook.call(context.self, context))
-          .then(mergeContext(context))
+      if (context.error !== error || context.result !== undefined) {
+        (context as any).original = { ...context };
+        context.error = error;
+        delete context.result;
       }
 
-      return promise.then(() => {
-        context.type = null;
-
-        if (context.result === undefined) {
+      return runHook(hook, context, 'error').then(() => {
+        if (context.result === undefined && context.error !== undefined) {
           throw context.error;
         }
       });
@@ -63,17 +42,26 @@ export function fromErrorHooks (hooks: LegacyHookFunction[]) {
   }
 }
 
-export function collectLegacyHooks (target: any, method: string) {
-  const {
-    before: { [method]: before = [] },
-    after: { [method]: after = [] },
-    error: { [method]: error = [] }
-  } = target.__hooks;
-  const beforeHooks = before;
-  const afterHooks = [...after].reverse();
-  const errorHook = fromErrorHooks(error);
+const RunHooks = <A, S> (hooks: LegacyHookFunction<A, S>[]) => (context: any) => {
+  return hooks.reduce((promise, hook) => {
+    return promise.then(() => runHook(hook, context))
+  }, Promise.resolve(undefined));
+};
 
-  return [errorHook, ...beforeHooks, ...afterHooks];
+export function fromBeforeHooks<A, S> (hooks: LegacyHookFunction<A, S>[]) {
+  return fromBeforeHook(RunHooks(hooks));
+}
+
+export function fromAfterHooks<A, S> (hooks: LegacyHookFunction<A, S>[]) {
+  return fromAfterHook(RunHooks(hooks));
+}
+
+export function fromErrorHooks<A, S> (hooks: LegacyHookFunction<A, S>[]) {
+  return fromErrorHook(RunHooks(hooks));
+}
+
+export function collectLegacyHooks (target: any, method: string) {
+  return target.__hooks.hooks[method] || [];
 }
 
 // Converts different hook registration formats into the
@@ -86,26 +74,36 @@ export function convertHookData (obj: any) {
   } else if (typeof obj !== 'object') {
     hook = { all: [ obj ] };
   } else {
-    each(obj, function (value, key) {
+    for (const [key, value] of Object.entries(obj)) {
       hook[key] = !Array.isArray(value) ? [ value ] : value;
-    });
+    }
   }
 
   return hook;
 }
 
+const types = ['before', 'after', 'error'];
+
+const wrappers: any = {
+  before: fromBeforeHooks,
+  after: fromAfterHooks,
+  error: fromErrorHooks,
+};
+
 // Add `.hooks` functionality to an object
 export function enableLegacyHooks (
   obj: any,
-  methods: string[] = ['find', 'get', 'create', 'update', 'patch', 'remove'],
-  types: string[] = ['before', 'after', 'error']
+  methods: string[] = defaultServiceMethods
 ) {
-  const hookData: any = {};
+  const hookData: any = {hooks: {}};
 
-  types.forEach(type => {
-    // Initialize properties where hook functions are stored
+  for (const type of types) {
     hookData[type] = {};
-  });
+  }
+
+  for (const method of methods) {
+    hookData.hooks[method] = [];
+  }
 
   // Add non-enumerable `__hooks` property to the object
   Object.defineProperty(obj, '__hooks', {
@@ -114,36 +112,49 @@ export function enableLegacyHooks (
     writable: true
   });
 
-  return function legacyHooks (this: any, allHooks: any) {
-    each(allHooks, (current: any, type) => {
-      if (!this.__hooks[type]) {
+  return function legacyHooks (this: any, allHooks: LegacyHookMap<any, any>) {
+    const touched = new Set<string>();
+
+    for (const [type, current] of Object.entries(allHooks)) {
+      if (!types.includes(type)) {
         throw new Error(`'${type}' is not a valid hook type`);
       }
 
       const hooks = convertHookData(current);
 
-      each(hooks, (_value, method) => {
-        if (method !== 'all' && methods.indexOf(method) === -1) {
+      for (const method of Object.keys(hooks)) {
+        if (method !== 'all' && !methods.includes(method)) {
           throw new Error(`'${method}' is not a valid hook method`);
         }
-      });
+      }
 
-      methods.forEach(method => {
-        let currentHooks = [...(hooks.all || []), ...(hooks[method] || [])];
+      for (const method of methods) {
+        if (!hooks.all?.length && !hooks[method]?.length) continue;
 
-        this.__hooks[type][method] = this.__hooks[type][method] || [];
+        const hook = this.__hooks[type][method] ||= (() => {
+          const hooks: LegacyHookFunction[] = [];
+          const hook = wrappers[type](hooks);
+          hook.hooks = hooks;
+          touched.add(method);
+          return hook;
+        })();
 
-        if (type === 'before') {
-          currentHooks = currentHooks.map(fromBeforeHook);
-        }
+        hook.hooks.push(...(hooks.all || []), ...(hooks[method] || []));
+      }
+    }
 
-        if (type === 'after') {
-          currentHooks = currentHooks.map(fromAfterHook);
-        }
+    for (const method of touched) {
+      const before = this.__hooks.before[method];
+      const after = this.__hooks.after[method];
+      const error = this.__hooks.error[method];
 
-        this.__hooks[type][method].push(...currentHooks);
-      });
-    });
+      const hooks: HookFunction[] = [];
+      if (error) hooks.push(error);
+      if (before) hooks.push(before);
+      if (after) hooks.push(after);
+
+      this.__hooks.hooks[method] = hooks;
+    }
 
     return this;
   }

packages/feathers/src/service.ts

@@ -24,6 +24,7 @@ export const defaultEventMap = {
 export const protectedMethods = Object.keys(Object.prototype)
   .concat(Object.keys(EventEmitter.prototype))
   .concat([
+    'all',
     'before',
     'after',
     'error',

packages/feathers/test/hooks/error.test.ts

@@ -11,7 +11,10 @@ describe('`error` hooks', () => {
     });
     const service = app.service('dummy');
 
-    afterEach(() => (service as any).__hooks.error.get = []);
+    afterEach(() => {
+      (service as any).__hooks.error.get = undefined;
+      (service as any).__hooks.hooks.get = [];
+    });
 
     it('basic error hook', async () => {
       service.hooks({