8000 fix: Clean up hooks code (#1407) · feathersjs/feathers@f25c88b · GitHub
[go: up one dir, main page]
Skip to content

Commit f25c88b

Browse files
vonagamdaffl
vonagam
authored and
daffl
committed
fix: Clean up hooks code (#1407)
1 parent f9d8536 commit f25c88b

File tree

6 files changed

+132
-61
lines changed

6 files changed

+132
-61
lines changed

‎packages/express/test/rest.test.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,8 @@ describe('@feathersjs/express/rest provider', () => {
249249
arguments: ['dishes', paramsWithHeaders ],
250250
type: 'error',
251251
method: 'get',
252-
path: 'hook-error'
252+
path: 'hook-error',
253+
original: data.hook.original
253254
},
254255
error: { message: 'I blew up' }
255256
});

‎packages/feathers/lib/hooks/base.js‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,14 @@ const { _ } = require('@feathersjs/commons');
33
const assignArguments = context => {
44
const { service, method } = context;
55
const parameters = service.methods[method];
6-
const argsObject = context.arguments.reduce((result, value, index) => {
7-
result[parameters[index]] = value;
8-
return result;
9-
}, {});
106

11-
if (!argsObject.params) {
12-
argsObject.params = {};
13-
}
7+
context.arguments.forEach((value, index) => {
8+
context[parameters[index]] = value;
9+
});
1410

15-
Object.assign(context, argsObject);
11+
if (!context.params) {
12+
context.params = {};
13+
}
1614

1715
return context;
1816
};

‎packages/feathers/lib/hooks/index.js‎

Lines changed: 66 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
const { hooks, isPromise, _ } = require('@feathersjs/commons');
1+
const { hooks, isPromise } = require('@feathersjs/commons');
22
const baseHooks = require('./base');
33

44
const {
@@ -9,11 +9,6 @@ const {
99
ACTIVATE_HOOKS
1010
} = hooks;
1111

12-
const makeArguments = (service, method, hookObject) => service.methods[ method ].reduce((result, value) => ([
13-
...result,
14-
hookObject[ value ]
15-
]), []);
16-
1712
const withHooks = function withHooks ({
1813
app,
1914
service,
@@ -33,8 +28,6 @@ const withHooks = function withHooks ({
3328
const returnHook = args[args.length - 1] === true
3429
? args.pop() : false;
3530

36-
// A reference to the original method
37-
const _super = original || service[method].bind(service);
3831
// Create the hook object that gets passed through
3932
const hookObject = createHookObject(method, {
4033
type: 'before', // initial hook object type
@@ -43,70 +36,93 @@ const withHooks = function withHooks ({
4336
app
4437
});
4538

46-
// Process all before hooks
47-
return processHooks.call(service, baseHooks.concat(hooks.before), hookObject)
48-
// Use the hook object to call the original method
39+
return Promise.resolve(hookObject)
40+
41+
// Run `before` hooks
42+
.then(hookObject => {
43+
return processHooks.call(service, baseHooks.concat(hooks.before), hookObject);
44+
})
45+
46+
// Run the original method
4947
.then(hookObject => {
5048
// If `hookObject.result` is set, skip the original method
5149
if (typeof hookObject.result !== 'undefined') {
5250
return hookObject;
5351
}
5452

5553
// Otherwise, call it with arguments created from the hook object
56-
const promise = _super(...makeArguments(service, method, hookObject));
57-
58-
if (!isPromise(promise)) {
59-
throw new Error(`Service method '${hookObject.method}' for '${hookObject.path}' service must return a promise`);
60-
}
54+
const promise = new Promise(resolve => {
55+
const func = original || service[method];
56+
const args = service.methods[method].map((value) => hookObject[value]);
57+
const result = func.apply(service, args);
6158

62-
return promise.then(result => {
63-
hookObject.result = result;
59+
if (!isPromise(result)) {
60+
throw new Error(`Service method '${hookObject.method}' for '${hookObject.path}' service must return a promise`);
61+
}
6462

65-
return hookObject;
63+
resolve(result);
6664
});
65+
66+
return promise
67+
.then(result => {
68+
hookObject.result = result;
69+
return hookObject;
70+
})
71+
.catch(error => {
72+
error.hook = hookObject;
73+
throw error;
74+
});
6775
})
68-
// Make a (shallow) copy of hookObject from `before` hooks and update type
69-
.then(hookObject => Object.assign({}, hookObject, { type: 'after' }))
70-
// Run through all `after` hooks
76+
77+
// Run `after` hooks
7178
.then(hookObject => {
72-
// Combine all app and service `after` and `finally` hooks and process
73-
const hookChain = hooks.after.concat(hooks.finally);
79+
const afterHookObject = Object.assign({}, hookObject, {
80+
type: 'after'
81+
});
7482

75-
return processHooks.call(service, hookChain, hookObject);
83+
return processHooks.call(service, hooks.after, afterHookObject);
7684
})
77-
.then(hookObject =>
78-
// Finally, return the result
79-
// Or the hook object if the `returnHook` flag is set
80-
returnHook ? hookObject : hookObject.result
81-
)
82-
// Handle errors
83-
.catch(error => {
84-
// Combine all app and service `error` and `finally` hooks and process
85-
const hookChain = hooks.error.concat(hooks.finally);
8685

87-
// A shallow copy of the hook object
88-
const errorHookObject = _.omit(Object.assign({}, error.hook, hookObject, {
86+
// Run `errors` hooks
87+
.catch(error => {
88+
const errorHookObject = Object.assign({}, error.hook, {
8989
type: 'error',
9090
original: error.hook,
91-
error
92-
}), 'result');
91+
error,
92+
result: undefined
93+
});
9394

94-
return processHooks.call(service, hookChain, errorHookObject)
95+
return processHooks.call(service, hooks.error, errorHookObject)
9596
.catch(error => {
96-
errorHookObject.error = error;
97+
const errorHookObject = Object.assign({}, error.hook, {
98+
error,
99+
result: undefined
100+
});
101+
102+
return errorHookObject;
103+
});
104+
})
105+
106+
// Run `finally` hooks
107+
.then(hookObject => {
108+
return processHooks.call(service, hooks.finally, hookObject)
109+
.catch(error => {
110+
const errorHookObject = Object.assign({}, error.hook, {
111+
error,
112+
result: undefined
113+
});
97114

98115
return errorHookObject;
99-
})
100-
.then(hook => {
101-
if (returnHook) {
102-
// Either resolve or reject with the hook object
103-
return typeof hook.result !== 'undefined' ? hook : Promise.reject(hook);
104-
}
105-
106-
// Otherwise return either the result if set (to swallow errors)
107-
// Or reject with the hook error
108-
return typeof hook.result !== 'undefined' ? hook.result : Promise.reject(hook.error);
109116
});
117+
})
118+
119+
// Resolve with a result or reject with an error
120+
.then(hookObject => {
121+
if (typeof hookObject.error !== 'undefined' && typeof hookObject.result === 'undefined') {
122+
return Promise.reject(returnHook ? hookObject : hookObject.error);
123+
} else {
124+
return returnHook ? hookObject : hookObject.result;
125+
}
110126
});
111127
};
112128
};

‎packages/feathers/test/hooks/error.test.js‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ describe('`error` hooks', () => {
1111
});
1212
const service = app.service('dummy');
1313

14-
afterEach(() => service.__hooks.error.get.pop());
14+
afterEach(() => service.__hooks.error.get = []);
1515

1616
it('basic error hook', () => {
1717
service.hooks({
@@ -154,6 +154,30 @@ describe('`error` hooks', () => {
154154
return app.service('dummy').get(1)
155155
.then(result => assert.strictEqual(result, null));
156156
});
157+
158+
it('uses the current hook object if thrown in a service method', () => {
159+
const app = feathers().use('/dummy', {
160+
get () {
161+
return Promise.reject(new Error('Something went wrong'));
162+
}
163+
});
164+
165+
const service = app.service('dummy');
166+
167+
service.hooks({
168+
before (hook) {
169+
return { ...hook, id: 42 };
170+
},
171+
error (hook) {
172+
assert.strictEqual(hook.id, 42);
173+
}
174+
});
175+
176+
return service.get(1).then(
177+
() => assert.fail('Should never get here'),
178+
error => assert.strictEqual(error.message, 'Something went wrong')
179+
);
180+
});
157181
});
158182

159183
describe('error in hooks', () => {

‎packages/feathers/test/hooks/finally.test.js‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,4 +73,36 @@ describe('`finally` hooks', () => {
7373
}
7474
);
7575
});
76+
77+
it('runs once, sets error if throws', () => {
78+
const app = feathers().use('/dummy', {
79+
get (id) {
80+
return Promise.resolve({ id });
81+
}
82+
});
83+
84+
const service = app.service('dummy');
85+
86+
let count = 0;
87+
88+
service.hooks({
89+
error (hook) {
90+
assert.fail('Should never get here (error hook)');
91+
},
92+
finally: [
93+
function (hook) {
94+
assert.strictEqual(++count, 1, 'This should be called only once');
95+
throw new Error('This did not work');
96+
},
97+
function (hook) {
98+
assert.fail('Should never get here (second finally hook)');
99+
}
100+
]
101+
});
102+
103+
return service.get(42).then(
104+
() => assert.fail('Should never get here (result resolve)'),
105+
error => assert.strictEqual(error.message, 'This did not work')
106+
);
107+
});
76108
});

‎packages/transport-commons/src/channels/mixins.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const ALL_EVENTS = Symbol('@feathersjs/transport-commons/all-events');
1111
export const keys = {
1212
PUBLISHERS: PUBLISHERS as typeof PUBLISHERS,
1313
CHANNELS: CHANNELS as typeof CHANNELS,
14-
ALL_EVENTS: ALL_EVENTS as typeof ALL_EVENTS,
14+
ALL_EVENTS: ALL_EVENTS as typeof ALL_EVENTS
1515
};
1616

1717
export interface ChannelMixin {

0 commit comments

Comments
 (0)
0