From f25c88bb86459989112d95f0eb4d3b2028b0bb56 Mon Sep 17 00:00:00 2001 From: Dmitrii Maganov Date: Sun, 30 Jun 2019 03:41:37 +0300 Subject: [PATCH] fix: Clean up hooks code (#1407) --- packages/express/test/rest.test.js | 3 +- packages/feathers/lib/hooks/base.js | 14 +-- packages/feathers/lib/hooks/index.js | 116 ++++++++++-------- packages/feathers/test/hooks/error.test.js | 26 +++- packages/feathers/test/hooks/finally.test.js | 32 +++++ .../transport-commons/src/channels/mixins.ts | 2 +- 6 files changed, 132 insertions(+), 61 deletions(-) diff --git a/packages/express/test/rest.test.js b/packages/express/test/rest.test.js index 702b5f37d3..f1eb65589d 100644 --- a/packages/express/test/rest.test.js +++ b/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' } }); diff --git a/packages/feathers/lib/hooks/base.js b/packages/feathers/lib/hooks/base.js index f8f07c044d..b044ea7983 100644 --- a/packages/feathers/lib/hooks/base.js +++ b/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; }; diff --git a/packages/feathers/lib/hooks/index.js b/packages/feathers/lib/hooks/index.js index 0817ada438..e1db2eb6f6 100644 --- a/packages/feathers/lib/hooks/index.js +++ b/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,9 +36,14 @@ 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') { @@ -53,60 +51,78 @@ const withHooks = function withHooks ({ } // 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; + } }); }; }; diff --git a/packages/feathers/test/hooks/error.test.js b/packages/feathers/test/hooks/error.test.js index a4e71a2644..a9539fcf8b 100644 --- a/packages/feathers/test/hooks/error.test.js +++ b/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', () => { diff --git a/packages/feathers/test/hooks/finally.test.js b/packages/feathers/test/hooks/finally.test.js index 72290de223..50dbd54180 100644 --- a/packages/feathers/test/hooks/finally.test.js +++ b/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') + ); + }); }); diff --git a/packages/transport-commons/src/channels/mixins.ts b/packages/transport-commons/src/channels/mixins.ts index b4101cf38a..a69b115c33 100644 --- a/packages/transport-commons/src/channels/mixins.ts +++ b/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 {