feat: Remove (hook, next) signature and SKIP support (#1269)

feathersjs · Apr 3, 2019 · 211c0f8 · 211c0f8
1 parent b487bbd
commit 211c0f8
Show file tree

Hide file tree

Showing 8 changed files with 75 additions and 220 deletions.
diff --git a/packages/authentication/lib/service.js b/packages/authentication/lib/service.js
@@ -56,7 +56,7 @@ module.exports = class AuthenticationService extends AuthenticationCore {
         if (authResult.accessToken) {
           return authResult;
         }
-        
+
         debug('Creating JWT with', payload, jwtOptions);
 
         return this.createJWT(payload, jwtOptions, params.secret)

diff --git a/packages/commons/lib/hooks.js b/packages/commons/lib/hooks.js
@@ -1,9 +1,5 @@
 const { _: { each, pick }, createSymbol } = require('./utils');
 
-// To skip further hooks
-const SKIP = createSymbol('__feathersSkipHooks');
-
-exports.SKIP = SKIP;
 exports.ACTIVATE_HOOKS = createSymbol('__feathersActivateHooks');
 
 exports.createHookObject = function createHookObject (method, data = {}) {
@@ -115,10 +111,6 @@ exports.processHooks = function processHooks (hooks, initialHookObject) {
     // Either use the returned hook object or the current
     // hook object from the chain if the hook returned undefined
     if (current) {
-      if (current === SKIP) {
-        return SKIP;
-      }
-
       if (!exports.isHookObject(current)) {
         throw new Error(`${hookObject.type} hook for '${hookObject.method}' method returned invalid hook object`);
       }
@@ -132,18 +124,8 @@ exports.processHooks = function processHooks (hooks, initialHookObject) {
   const promise = hooks.reduce((promise, fn) => {
     const hook = fn.bind(this);
 
-    if (hook.length === 2) { // function(hook, next)
-      promise = promise.then(hookObject => hookObject === SKIP ? SKIP : new Promise((resolve, reject) => {
-        hook(hookObject, (error, result) =>
-          error ? reject(error) : resolve(result)
-        );
-      }));
-    } else { // function(hook)
-      promise = promise.then(hookObject => hookObject === SKIP ? SKIP : hook(hookObject));
-    }
-
     // Use the returned hook object or the old one
-    return promise.then(updateCurrentHook);
+    return promise.then(hookObject => hook(hookObject)).then(updateCurrentHook);
   }, Promise.resolve(hookObject));
 
   return promise.then(() => hookObject).catch(error => {

diff --git a/packages/commons/test/hooks.test.js b/packages/commons/test/hooks.test.js
@@ -274,67 +274,14 @@ describe('hook utilities', () => {
           return Promise.resolve(hook);
         },
 
-        function (hook, next) {
-          hook.chain.push('second');
-
-          next();
-        },
-
         hook => {
-          hook.chain.push('third');
-        },
-
-        function (hook) {
-          hook.chain.push('fourth');
-
-          return hook;
-        }
-      ], dummyHook);
-
-      return promise.then(result => {
-        expect(result).to.deep.equal({
-          type: 'dummy',
-          method: 'something',
-          chain: [ 'first', 'second', 'third', 'fourth' ]
-        });
-      });
-    });
-
-    it('skip further hooks', () => {
-      const dummyHook = {
-        type: 'dummy',
-        method: 'something'
-      };
-
-      const promise = hooks.processHooks([
-        function (hook) {
-          hook.chain = [ 'first' ];
-
-          return Promise.resolve(hook);
-        },
-
-        function (hook, next) {
           hook.chain.push('second');
-
-          next();
-        },
-
-        hook => {
-          hook.chain.push('third');
-
-          return hooks.SKIP;
         },
 
         function (hook) {
-          hook.chain.push('fourth');
+          hook.chain.push('third');
 
           return hook;
-        },
-
-        function (hook, next) {
-          hook.chain.push('fourth');
-
-          next();
         }
       ], dummyHook);
 
@@ -347,30 +294,6 @@ describe('hook utilities', () => {
       });
     });
 
-    it('next and next with error', () => {
-      const dummyHook = {
-        type: 'dummy',
-        method: 'something'
-      };
-
-      const promise = hooks.processHooks([
-        function (hook, next) {
-          hook.test = 'first ran';
-
-          next();
-        },
-
-        function (hook, next) {
-          next(new Error('This did not work'));
-        }
-      ], dummyHook);
-
-      return promise.catch(error => {
-        expect(error.message).to.equal('This did not work');
-        expect(error.hook.test).to.equal('first ran');
-      });
-    });
-
     it('errors when invalid hook object is returned', () => {
       const dummyHook = {
         type: 'dummy',

diff --git a/packages/feathers/lib/index.js b/packages/feathers/lib/index.js
@@ -1,4 +1,3 @@
-const { hooks } = require('@feathersjs/commons');
 const Proto = require('uberproto');
 const Application = require('./application');
 const version = require('./version');
@@ -19,7 +18,6 @@ function createApplication () {
 }
 
 createApplication.version = version;
-createApplication.SKIP = hooks.SKIP;
 createApplication.ACTIVATE_HOOKS = ACTIVATE_HOOKS;
 createApplication.activateHooks = activateHooks;
 

diff --git a/packages/feathers/test/application.test.js b/packages/feathers/test/application.test.js
@@ -1,6 +1,5 @@
 const assert = require('assert');
 const Proto = require('uberproto');
-const { hooks } = require('@feathersjs/commons');
 const feathers = require('../lib');
 
 describe('Feathers application', () => {
@@ -23,10 +22,6 @@ describe('Feathers application', () => {
     assert.strictEqual(app.version, '3.0.0-development');
   });
 
-  it('sets SKIP on main', () => {
-    assert.strictEqual(feathers.SKIP, hooks.SKIP);
-  });
-
   it('is an event emitter', done => {
     const app = feathers();
     const original = { hello: 'world' };

diff --git a/packages/feathers/test/hooks/after.test.js b/packages/feathers/test/hooks/after.test.js
@@ -120,12 +120,12 @@ describe('`after` hooks', () => {
 
       service.hooks({
         after: {
-          create (hook, next) {
+          create (hook) {
             assert.strictEqual(hook.type, 'after');
 
             hook.result.some = 'thing';
 
-            next(null, hook);
+            return hook;
           }
         }
       });
@@ -147,11 +147,11 @@ describe('`after` hooks', () => {
 
       service.hooks({
         after: {
-          create (hook, next) {
+          create (hook) {
             hook.result.appPresent = typeof hook.app !== 'undefined';
             assert.strictEqual(hook.result.appPresent, true);
 
-            next(null, hook);
+            return hook;
           }
         }
       });
@@ -173,8 +173,8 @@ describe('`after` hooks', () => {
 
       service.hooks({
         after: {
-          update (hook, next) {
-            next(new Error('This did not work'));
+          update () {
+            throw new Error('This did not work');
           }
         }
       });
@@ -221,19 +221,18 @@ describe('`after` hooks', () => {
 
       service.hooks({
         after: {
-          create (hook, next) {
+          create (hook) {
             hook.result.some = 'thing';
-            next();
+
+            return hook;
           }
         }
       });
 
       service.hooks({
         after: {
-          create (hook, next) {
+          create (hook) {
             hook.result.other = 'stuff';
-
-            next();
           }
         }
       });
@@ -260,14 +259,15 @@ describe('`after` hooks', () => {
       service.hooks({
         after: {
           create: [
-            function (hook, next) {
+            function (hook) {
               hook.result.some = 'thing';
-              next();
+
+              return hook;
             },
-            function (hook, next) {
+            function (hook) {
               hook.result.other = 'stuff';
 
-              next();
+              return hook;
             }
           ]
         }
@@ -293,23 +293,26 @@ describe('`after` hooks', () => {
 
       service.hooks({
         after: {
-          get (hook, next) {
+          get (hook) {
             hook.result.items = ['first'];
-            next();
+
+            return hook;
           }
         }
       });
 
       service.hooks({
         after: {
           get: [
-            function (hook, next) {
+            function (hook) {
               hook.result.items.push('second');
-              next();
+
+              return hook;
             },
-            function (hook, next) {
+            function (hook) {
               hook.result.items.push('third');
-              next();
+
+              return hook;
             }
           ]
         }
@@ -338,18 +341,20 @@ describe('`after` hooks', () => {
 
       service.hooks({
         after: {
-          all (hook, next) {
+          all (hook) {
             hook.result.afterAllObject = true;
-            next();
+
+            return hook;
           }
         }
       });
 
       service.hooks({
         after: [
-          function (hook, next) {
+          function (hook) {
             hook.result.afterAllMethodArray = true;
-            next();
+
+            return hook;
           }
         ]
       });
@@ -380,9 +385,10 @@ describe('`after` hooks', () => {
 
       service.hooks({
         after: {
-          get (hook, next) {
+          get (hook) {
             hook.result.test = this.number + 1;
-            next();
+
+            return hook;
           }
         }
       });
@@ -395,32 +401,5 @@ describe('`after` hooks', () => {
         });
       });
     });
-
-    it('can not call next() multiple times', () => {
-      const app = feathers().use('/dummy', {
-        get (id) {
-          return Promise.resolve({ id });
-        }
-      });
-
-      const service = app.service('dummy');
-
-      service.hooks({
-        after: {
-          get: [
-            function (hook, next) {
-              next();
-            },
-
-            function (hook, next) {
-              next();
-              next();
-            }
-          ]
-        }
-      });
-
-      return service.get(10);
-    });
   });
 });