Open hook workflow to custom methods

[bertho-zero]

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:

const app = feathers().use('/dummy', {
  methods: {
    custom: ['id', 'data', 'params']
  },
  get() {},
  // ...
  custom(id, data, params) {
    return Promise.resolve([id, data, params]);
  }
});

app.service('dummy').hooks({
  before: {
    all: [/**/],
    custom: [/**/]
  },
  after: {
    all: [/**/],
    custom: [/**/]
  }
});

app.service('dummy').custom(28, { bar: 'foo' }, { provider: 'rest' });

[bertho-zero]

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 });
}));

[bertho-zero]

These changes are not breaking changes because service.methods did not exist before.

[bertho-zero]

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/commons

Index: 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/feathers

Index: 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']);
+    });
   });
 });

[bertho-zero]

I think that all methods can be added to service.methods, for example the argument list can be used in @feathersjs/express to convert the request into arguments.

https://github.com/feathersjs/express/blob/9bc5541107bfd1ec58d3e38fb968b91832cda377/lib/rest/wrappers.js#L111-L116

[daffl]

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?

[bertho-zero]

I just had to rename activeHooks to activateHooks, it's all good.
Just publish a version with the latest @feathers/commons changes and tests pass.

[bertho-zero]

In @feathersjs/express for example, we could use a function like

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 argsOrder comes from service.methods.

Using the Symbols implies that you need a require of @feathersjs/feathers to compare with the original Symbol, perhaps an https://docs.npmjs.com/files/package.json#optionaldependencies.
Symbols can only be used in @feathersjs/feathersjs if you pass all methods by service.methods (not just customs)

[daffl]

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).

[bertho-zero]

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.
In other cases your services will have a __feathersActivateHooks property.

[bertho-zero]

@daffl Perhaps you could release a major beta version of @feathersjs/common and a version of @feathersjs/feathers on a tag that is not "latest" ? It will allow to test that in production.

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.

[daffl]

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?

[bertho-zero]

#875 is apart, I'll update it after.

[daffl]

They should go out in the same version though right?

[bertho-zero]

Not necessarily, It can go out in another version.
I'll update it next week.