Allows error hooks to swallow error by setting the result (#621)

lib/hooks.js

@@ -92,13 +92,22 @@ const hookMixin = exports.hookMixin = function hookMixin (service) {
 
           const errorHookObject = Object.assign({}, error.hook || hookObject, {
             type: 'error',
+            result: null,
             original: error.hook,
             error
           });
 
           return processHooks
             .call(service, hookChain, errorHookObject)
-            .then(hook => Promise.reject(hook.error));
+            .then(hook => {
+              if (errorHookObject.params.__returnHook) {
+                return Promise.reject(hook);
+              } else if (hook.result) {
+                return Promise.resolve(hook.result);
+              }
+
+              return Promise.reject(hook.error);
+            });
         });
     };
   });

test/hooks/after.test.js

@@ -185,7 +185,7 @@ describe('`after` hooks', () => {
       });
     });
 
-    it('does not run after hook when there is an error', done => {
+    it('does not run after hook when there is an error', () => {
       const dummyService = {
         remove () {
           return Promise.reject(new Error('Error removing item'));
@@ -203,10 +203,9 @@ describe('`after` hooks', () => {
         }
       });
 
-      service.remove(1, {}).catch(error => {
+      return service.remove(1, {}).catch(error => {
         assert.ok(error, 'Got error');
         assert.equal(error.message, 'Error removing item', 'Got error message from service');
-        done();
       });
     });
 

test/hooks/error.test.js

@@ -13,7 +13,7 @@ describe('`error` hooks', () => {
 
     afterEach(() => service.__hooks.error.get.pop());
 
-    it('basic error hook', done => {
+    it('basic error hook', () => {
       service.hooks({
         error: {
           get (hook) {
@@ -26,9 +26,9 @@ describe('`error` hooks', () => {
         }
       });
 
-      service.get('test').then(() => {
-        done(new Error('Should never get here'));
-      }).catch(() => done());
+      return service.get('test').then(() => {
+        throw new Error('Should never get here');
+      }).catch(() => true);
     });
 
     it('can change the error', () => {
@@ -118,6 +118,23 @@ describe('`error` hooks', () => {
         assert.ok(error.third);
       });
     });
+
+    it('setting `hook.result` will return result', () => {
+      const data = {
+        message: 'It worked'
+      };
+
+      service.hooks({
+        error: {
+          get (hook) {
+            hook.result = data;
+          }
+        }
+      });
+
+      return service.get(10)
+        .then(result => assert.deepEqual(result, data));
+    });
   });
 
   describe('error in hooks', () => {
@@ -137,7 +154,7 @@ describe('`error` hooks', () => {
       service = app.service('dummy');
     });
 
-    it('error in before hook', done => {
+    it('in before hook', () => {
       service.hooks({
         before () {
           throw new Error(errorMessage);
@@ -149,14 +166,17 @@ describe('`error` hooks', () => {
           );
           assert.equal(hook.id, 'dishes');
           assert.equal(hook.error.message, errorMessage);
-          done();
         }
       });
 
-      service.get('dishes').then(done);
+      return service.get('dishes')
+        .then(() => {
+          throw new Error('Should never get here');
+        })
+        .catch(error => assert.equal(error.message, errorMessage));
     });
 
-    it('error in after hook', done => {
+    it('in after hook', () => {
       service.hooks({
         after () {
           throw new Error(errorMessage);
@@ -167,19 +187,22 @@ describe('`error` hooks', () => {
             'Original hook still set'
           );
           assert.equal(hook.id, 'dishes');
-          assert.deepEqual(hook.result, {
+          assert.deepEqual(hook.original.result, {
             id: 'dishes',
             text: 'You have to do dishes'
           });
           assert.equal(hook.error.message, errorMessage);
-          done();
         }
       });
 
-      service.get('dishes').then(done);
+      return service.get('dishes')
+        .then(() => {
+          throw new Error('Should never get here');
+        })
+        .catch(error => assert.equal(error.message, errorMessage));
     });
 
-    it('uses the current hook object if thrown in a hook and sets hook.original', done => {
+    it('uses the current hook object if thrown in a hook and sets hook.original', () => {
       service.hooks({
         after (hook) {
           hook.modified = true;
@@ -190,12 +213,14 @@ describe('`error` hooks', () => {
         error (hook) {
           assert.ok(hook.modified);
           assert.equal(hook.original.type, 'after');
-
-          done();
         }
       });
 
-      service.get('laundry').then(done);
+      return service.get('laundry')
+        .then(() => {
+          throw new Error('Should never get here');
+        })
+        .catch(error => assert.equal(error.message, errorMessage));
     });
   });
 });

test/hooks/hooks.test.js

@@ -45,7 +45,7 @@ describe('hooks basics', () => {
     });
   });
 
-  it('has hook.app, hook.service and hook.path', done => {
+  it('has hook.app, hook.service and hook.path', () => {
     const app = feathers().use('/dummy', {
       get (id) {
         return Promise.resolve({ id });
@@ -56,19 +56,14 @@ describe('hooks basics', () => {
 
     service.hooks({
       before (hook) {
-        try {
-          assert.equal(this, service);
-          assert.equal(hook.service, service);
-          assert.equal(hook.app, app);
-          assert.equal(hook.path, 'dummy');
-          done();
-        } catch (e) {
-          done(e);
-        }
+        assert.equal(this, service);
+        assert.equal(hook.service, service);
+        assert.equal(hook.app, app);
+        assert.equal(hook.path, 'dummy');
       }
     });
 
-    service.get('test');
+    return service.get('test');
   });
 
   it('does not error when result is null', () => {
@@ -153,23 +148,43 @@ describe('hooks basics', () => {
     });
   });
 
-  it('allows to return the hook object', () => {
-    const app = feathers().use('/dummy', {
-      get (id, params) {
-        return Promise.resolve({ id, params });
-      }
+  describe('returns the hook object with __returnHook', () => {
+    it('on normal method call', () => {
+      const app = feathers().use('/dummy', {
+        get (id, params) {
+          return Promise.resolve({ id, params });
+        }
+      });
+      const params = {
+        __returnHook: true
+      };
+
+      return app.service('dummy').get(10, params).then(context => {
+        assert.equal(context.service, app.service('dummy'));
+        assert.equal(context.type, 'after');
+        assert.equal(context.path, 'dummy');
+        assert.deepEqual(context.result, {
+          id: 10,
+          params
+        });
+      });
     });
-    const params = {
-      __returnHook: true
-    };
-
-    return app.service('dummy').get(10, params).then(context => {
-      assert.equal(context.service, app.service('dummy'));
-      assert.equal(context.type, 'after');
-      assert.equal(context.path, 'dummy');
-      assert.deepEqual(context.result, {
-        id: 10,
-        params
+
+    it('on error', () => {
+      const app = feathers().use('/dummy', {
+        get () {
+          return Promise.reject(new Error('Something went wrong'));
+        }
+      });
+      const params = {
+        __returnHook: true
+      };
+
+      return app.service('dummy').get(10, params).catch(context => {
+        assert.equal(context.service, app.service('dummy'));
+        assert.equal(context.type, 'error');
+        assert.equal(context.path, 'dummy');
+        assert.equal(context.error.message, 'Something went wrong');
       });
     });
   });