From 211c0f86634b13b234471be6518f7a91bae781ca Mon Sep 17 00:00:00 2001 From: David Luecke Date: Wed, 3 Apr 2019 10:29:28 -0700 Subject: [PATCH] feat: Remove (hook, next) signature and SKIP support (#1269) --- packages/authentication/lib/service.js | 2 +- packages/commons/lib/hooks.js | 20 +---- packages/commons/test/hooks.test.js | 79 +----------------- packages/feathers/lib/index.js | 2 - packages/feathers/test/application.test.js | 5 -- packages/feathers/test/hooks/after.test.js | 87 ++++++++----------- packages/feathers/test/hooks/before.test.js | 92 ++++++++------------- packages/feathers/test/hooks/error.test.js | 8 +- 8 files changed, 75 insertions(+), 220 deletions(-) diff --git a/packages/authentication/lib/service.js b/packages/authentication/lib/service.js index b459a3e8a2..9984c1617b 100644 --- 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 index ed50a9bbf5..45c4a0c5f1 100644 --- 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 index 231db857b7..a84538ba59 100644 --- 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 index 82e2ff2700..33acf66abe 100644 --- 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 index cb43f65cf1..b1cfc390ed 100644 --- 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 index 83377a5d23..4b038677f6 100644 --- 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,9 +293,10 @@ describe('`after` hooks', () => { service.hooks({ after: { - get (hook, next) { + get (hook) { hook.result.items = ['first']; - next(); + + return hook; } } }); @@ -303,13 +304,15 @@ describe('`after` hooks', () => { 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); - }); }); }); diff --git a/packages/feathers/test/hooks/before.test.js b/packages/feathers/test/hooks/before.test.js index a53455615e..18955545d9 100644 --- a/packages/feathers/test/hooks/before.test.js +++ b/packages/feathers/test/hooks/before.test.js @@ -182,7 +182,7 @@ describe('`before` hooks', () => { service.hooks({ before: { - create (hook, next) { + create (hook) { assert.strictEqual(hook.type, 'before'); hook.data.modified = 'data'; @@ -191,7 +191,7 @@ describe('`before` hooks', () => { modified: 'params' }); - next(null, hook); + return hook; } } }); @@ -216,10 +216,11 @@ describe('`before` hooks', () => { someService.hooks({ before: { - create (hook, next) { + create (hook) { hook.data.appPresent = typeof hook.app !== 'undefined'; assert.strictEqual(hook.data.appPresent, true); - next(null, hook); + + return hook; } } }); @@ -244,8 +245,8 @@ describe('`before` hooks', () => { service.hooks({ before: { - update (hook, next) { - next(new Error('You are not allowed to update')); + update () { + throw new Error('You are not allowed to update'); } } }); @@ -271,8 +272,8 @@ describe('`before` hooks', () => { service.hooks({ before: { - remove (hook, next) { - next(); + remove (hook) { + return hook; } } }); @@ -301,20 +302,20 @@ describe('`before` hooks', () => { service.hooks({ before: { - create (hook, next) { + create (hook) { hook.params.modified = 'params'; - next(); + return hook; } } }); service.hooks({ before: { - create (hook, next) { + create (hook) { hook.data.modified = 'second data'; - next(); + return hook; } } }); @@ -344,15 +345,15 @@ describe('`before` hooks', () => { service.hooks({ before: { create: [ - function (hook, next) { + function (hook) { hook.params.modified = 'params'; - next(); + return hook; }, - function (hook, next) { + function (hook) { hook.data.modified = 'second data'; - next(); + return hook; } ] } @@ -377,9 +378,10 @@ describe('`before` hooks', () => { service.hooks({ before: { - get (hook, next) { + get (hook) { hook.params.items = ['first']; - next(); + + return hook; } } }); @@ -387,13 +389,15 @@ describe('`before` hooks', () => { service.hooks({ before: { get: [ - function (hook, next) { + function (hook) { hook.params.items.push('second'); - next(); + + return hook; }, - function (hook, next) { + function (hook) { hook.params.items.push('third'); - next(); + + return hook; } ] } @@ -426,18 +430,20 @@ describe('`before` hooks', () => { service.hooks({ before: { - all (hook, next) { + all (hook) { hook.params.beforeAllObject = true; - next(); + + return hook; } } }); service.hooks({ before: [ - function (hook, next) { + function (hook) { hook.params.beforeAllMethodArray = true; - next(); + + return hook; } ] }); @@ -461,9 +467,10 @@ describe('`before` hooks', () => { service.hooks({ before: { - get (hook, next) { + get (hook) { hook.params.test = this.number + 2; - next(); + + return hook; } } }); @@ -476,34 +483,5 @@ describe('`before` hooks', () => { }); }); }); - - it('calling next() multiple times does not do anything', () => { - const app = feathers().use('/dummy', { - get (id) { - return Promise.resolve({ id }); - } - }); - - const service = app.service('dummy'); - - service.hooks({ - before: { - get: [ - function (hook, next) { - next(); - }, - - function (hook, next) { - next(); - next(); - } - ] - } - }); - - return service.get(10).then(data => { - assert.deepStrictEqual(data, { id: 10 }); - }); - }); }); }); diff --git a/packages/feathers/test/hooks/error.test.js b/packages/feathers/test/hooks/error.test.js index 58ec8a2996..51b5fd57d6 100644 --- a/packages/feathers/test/hooks/error.test.js +++ b/packages/feathers/test/hooks/error.test.js @@ -76,8 +76,8 @@ describe('`error` hooks', () => { it('calling `next` with error', () => { service.hooks({ error: { - get (hook, next) { - next(new Error(errorMessage)); + get () { + throw new Error(errorMessage); } } }); @@ -102,10 +102,10 @@ describe('`error` hooks', () => { return Promise.resolve(hook); }, - function (hook, next) { + function (hook) { hook.error.third = true; - next(); + return hook; } ] }