-
-
Notifications
You must be signed in to change notification settings - Fork 752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open hook workflow to custom methods #864
Conversation
And then we can take advantage of authentication, validation, formatting, parsing, etc. without any other effort: // for async/await in express middleware
const wrap = fn => (req, res, next) => Promise.resolve(fn(req, res, next)).catch(next);
app.use('/users/:id/custom', wrap(async (req, res) => {
const result = await app.service('dummy')
.custom(req.params.id, req.body, {
query: req.query,
provider: 'rest'
});
res.send({ result });
})); |
These changes are not breaking changes because |
I just thought of another syntax: const app = feathers().use('/dummy', {
get() {},
// ...
@activeHooks(['id', 'data', 'params'])
custom(id, data, params) {
return Promise.resolve([id, data, params]);
}
});
// or without decorator
const app = feathers().use('/dummy', {
get() {},
// ...
custom: activeHooks(['id', 'data', 'params'])(
(id, data, params) => {
return Promise.resolve([id, data, params]);
}
)
}); Which do you prefer between this one and that of the PR? EDIT: I have no opinion but the two could easily be combined. The needed changes for the an 'activeHooks' methods
@feathersjs/commonsIndex: lib/hooks.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- lib/hooks.js (date 1527341880000)
+++ lib/hooks.js (date 1527952294000)
@@ -5,6 +5,17 @@
? Symbol('__feathersSkipHooks')
: '__feathersSkipHooks';
+const ACTIVE_HOOKS = exports.ACTIVE_HOOKS = typeof Symbol !== 'undefined'
+ ? Symbol('__feathersActiveHooks')
+ : '__feathersActiveHooks';
+
+exports.activeHooks = function activeHooks (args) {
+ return fn => {
+ fn[ACTIVE_HOOKS] = args;
+ return fn;
+ };
+};
+
exports.createHookObject = function createHookObject (method, data = {}) {
const hook = {};
@feathersjs/feathersIndex: lib/hooks.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- lib/hooks.js (revision 390abfc24dd3fd3f030e5278fc4b92371feeb59c)
+++ lib/hooks.js (date 1527952294000)
@@ -24,7 +24,14 @@
}
const app = this;
- const methods = Object.assign({}, argumentsOrders, service.methods || {});
+ const serviceMethods = service.methods || {};
+ const activatedMethods = Object.keys(service)
+ .filter(key => typeof service[key] === 'function' && service[key][hooks.ACTIVE_HOOKS])
+ .reduce((result, methodName) => {
+ result[methodName] = service[methodName][hooks.ACTIVE_HOOKS];
+ return result;
+ }, serviceMethods);
+ const methods = Object.assign({}, argumentsOrders, activatedMethods);
const methodNames = Object.keys(methods);
const mixin = {};
Index: lib/index.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- lib/index.js (revision 390abfc24dd3fd3f030e5278fc4b92371feeb59c)
+++ lib/index.js (date 1527952316000)
@@ -16,6 +16,8 @@
createApplication.version = version;
createApplication.SKIP = hooks.SKIP;
+createApplication.ACTIVE_HOOKS = hooks.ACTIVE_HOOKS;
+createApplication.activeHooks = hooks.activeHooks;
module.exports = createApplication;
Index: test/hooks/hooks.test.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- test/hooks/hooks.test.js (revision 390abfc24dd3fd3f030e5278fc4b92371feeb59c)
+++ test/hooks/hooks.test.js (date 1527952522000)
@@ -243,7 +243,7 @@
});
});
- it('can register hooks on a custom method', () => {
+ it('can register hooks on a custom method', async () => {
const app = feathers().use('/dummy', {
methods: {
custom: ['id', 'data', 'params']
@@ -251,7 +251,13 @@
get () {},
custom (id, data, params) {
return Promise.resolve([id, data, params]);
- }
+ },
+ // ActiveHooks is usable as a decorator: @activeHooks(['id', 'data', 'params'])
+ other: feathers.activeHooks(['id', 'data', 'params'])(
+ (id, data, params) => {
+ return Promise.resolve([id, data, params]);
+ }
+ )
});
app.service('dummy').hooks({
@@ -275,9 +281,19 @@
const args = [1, { test: 'ok' }, { provider: 'rest' }];
- return app.service('dummy').custom(...args, true).then(hook => {
+ assert.deepEqual(app.service('dummy').methods, {
+ custom: ['id', 'data', 'params'],
+ other: ['id', 'data', 'params']
+ });
+
+ await app.service('dummy').custom(...args, true).then(hook => {
assert.deepEqual(hook.result, args);
assert.deepEqual(hook.test, ['all::before', 'custom::before', 'all::after', 'custom::after']);
});
+
+ return app.service('dummy').other(...args, true).then(hook => {
+ assert.deepEqual(hook.result, args);
+ assert.deepEqual(hook.test, ['all::before', 'all::after']);
+ });
});
});
|
I think that all methods can be added to |
I like the const app = feathers().use('/dummy', {
get() {},
// ...
custom: activateHooks(['id', 'data', 'params'])(
(id, data, params) => {
return Promise.resolve([id, data, params]);
}
)
}); Syntax. Now that you got Symbols working, is there anything that would change? |
I just had to rename |
In function makeArgsGetter (argsOrder) {
return (req, params) => argsOrder.reduce((result, argName) => {
switch (argName) {
case 'id':
return [ ...result, req.params.__feathersId || null ];
case 'data':
return [ ...result, req.body ];
case 'params':
return [ ...result, params ];
}
}, []);
} where Using the Symbols implies that you need a require of |
I'll review this again over the weekend. The problem with Symbols is that they may not work in the browser (although I think that the Polyfill Feathers requires should work for most). |
Thank you. Symbols have polyfills and feathers no longer explicitly require babel-polyfill but we can put a note for those who want to use this feature. Symbols are not compatible with all browsers, be sure to precompile your code or add a Polyfill for older browsers. |
@daffl Perhaps you could release a major beta version of This change is a new feature without regression or change of API, it could even be published in a patch version without waiting for the Crow version. |
You are totally right. Sorry I'm so slow - that happens with me sometimes during summertime 😉 - I just released a version 2.0.0 of https://github.com/feathersjs/commons. Could you update the pull request pointing to that version and fix the conflicts? Then everything should pass right? How does #875 fit in here? |
#875 is apart, I'll update it after. |
They should go out in the same version though right? |
Not necessarily, It can go out in another version. |
Works with feathersjs-ecosystem/commons#72.
This PR allows you to add converters to your custom methods, which allows to create the hook corresponding to your arguments and finally to fully use the hook workflow.
I would have liked to use a Symbol but #863.
Example: