feathersjs / feathers

The API and real-time application framework
https://feathersjs.com
MIT License
15.06k stars 752 forks source link

FEATURE PROPOSAL: fine-grained control over the hook chain #816

Open claustres opened 6 years ago

claustres commented 6 years ago

Problem to be solved

Feathers allow to register hooks on a service using service.hooks({ xxx }), however this is a append-only method. Once some hooks have been registered it is impossible to insert new hooks into the hook chain.

Use case

When you create a modular application each module usually exposes a set of services with associated hooks. Then the application add specific hooks over the existing services to customize the behaviour. It is often required that the application can control the hook order to ensure the correct behaviour.

Example: module A exposes a user service with a built-in before hook to hash password on user creation. Application wants to be able to automatically create users with generated passwords under certain conditions, it should then be able to insert a password generation hook before the hash password hook in user service.

A solution is to not expose hooks in modules and plug all hooks at the application level. However this means your application becomes more monolithic, you cannot develop each module independently from your application and this breaks somehow the module encapsulation.

Proposed API

The hook chain for a given hook type is basically an array, so we need a function to insert a hook at a given position or before another one given its name (this mean we should have named hooks), e.g. service.hooks.before().all().insert(hook, index) or service.hooks.after().find().insert(hook, 'hook_name') where hook could be an object like this:

{
  name: 'new_hook_name',
  hook: standard Feathers hook function
}

This article can be used as a source of inspiration.

Open questions

When all the services are located in the same process everything is fine with the proposed API, however if you think about microservices distributed across different machines it is unlikely you can alter the code of a remote process by inserting a hook function. In this case we should consider the transparent remote execution of hooks, like this is done for services in https://github.com/kalisio/feathers-distributed.

daffl commented 6 years ago

Personally, I am a fan of being explicitly able to see the entire hook flow in a single file (in hindsight, even being able call .hooks multiple times probably wasn't ideal) even if it can sometimes be a little verbose.

In my experience features like this - although well intentioned - add a lot of complexity and unpredictability. I just made the same mistake again the other day, trying to add hooks automatically when it eventually turned out that you want to do things before or after those explicitly. Now the library just exports them and you have to put them where you want them to use explicitly and it is obvious to everybody going into the codebase what is happening.

On a technical side, I think what you are suggesting can be implemented as a plugin using some of the existing hook helpers. Something like this might help to get started:

const { enableHooks, processHooks } = require('@feathersjs/commons').hooks;

class CustomHookChain {
  constructor(app) {
    // Adds a `this.__hooks` with all methods and types
    // and a `.hooks` method that works like the normal ones
    enableHooks(this, app.methods, app.hookTypes);
  }

  prepend(type, method, ...hooks) {
    const myHooks = this.__hooks[type][method];

    if(!myHooks.length) {
      // If there is no hooks yet we can just register them
      this.hooks({
        [type]: {
          [method]: hooks
        }
      });
    } else {
      myHooks.unshift(hooks);
    }
  }

  getHook() {
    return context => {
      const myHooks = this.__hooks[context.type][context.method];

      return processHooks(myHooks, context);
    }
  }
}

module.exports = app => {
  app.mixins.push(service => {
    service.hookChain = new hookChain(app);
    service.hooks(service.hookChain.getHook());
  });
}

Now you can add and use your own hook flow functionality in app.service('myservice').hookChain.hooks(), app.service('myservice').hookChain.prepend() etc.

claustres commented 6 years ago

You are fundamentally right, having everything in one place is the best, and when things tend to depend on some ordering yes unpredictability can occur when adding features. However in practice things are often not that easy and ordering can be required. When favoring a modular and micro-service approach you also have to implement self-sufficient pieces of code and you cannot make such a strong coupling or it will be harder to work independently and scale horizontally. Last but not least, hooks are a way to customise a Feathers DB service (maybe there is a better one let me know) so that the pair DB service + service hooks can be seen from an external point of view as a single feature service. This feature service only requires the service hooks to be present in the right order to work correctly but will be insensitive to additional custom hooks that an app could add.

I do not say I have the right solution but I still consider that having more control over the hook chain will benefit some use cases. Thanks for your code sample.

By the way we are planning to develop a tool to generate a hook diagram from a hook file so that you have a clear view of your app workflow (any help/idea is welcome ;-).

claustres commented 6 years ago

Just to be more clear, if the app holds all hooks then it is also required to hold all the services. In a microservice deployment this raises two issues I think:

leob commented 6 years ago

Interesting topic ... never thought about 'modules' before but I share some of these concerns, I'm a bit afraid that for a larger Feathers application the amount of hooks config can become unwieldy and hard to manage. Maybe some simple helper functions are the answer, maybe relying not solely on hooks but also on service composition, maybe some other patterns which I don't know about yet ... what we might need is a library of architecture 'patterns' which show how to put together a Feathers app, especially a larger one. I'm trying out different things and still finding my way around Feathers, if I cook up anything interesting I might put it in a Gist or a github repo.

StevenBlack commented 6 years ago

Just seeing this now, jumping in, FWIW.

I've worked so much with hooks over the years, all sorts of scenarios and implementations.

Here's a concept for a thought experiment: Arguably, Hooks should be typed. Once this happens, many implementation scenarios are possible.

Consider for example that an Anchor is a type of Hook. Makes semantic sense, right?

An Anchor, depending on its implementation, can

It's strictly not that easy. Some key issues include error handling, and how to handle Promises.

One way to handle errors more cleanly is to make anchors responsible for handling errors from within its hook chains and collections. Promise issues fall away if you arbitrarily declare, at the outset, everything is a Promise.

You can do a lot with hooks if you give them chops. One example: https://github.com/StevenBlack/hooks-and-anchors. Warning: I created this while using a now-ancient version of Feathers, which pre-dates async/await. I'm just now working on legacy code, and considering upgrading this to Feathers 3, feeling like a noob again.

leob commented 6 years ago

Pretty brilliant ... yes a way to compose hooks sounds very useful.

claustres commented 6 years ago

That's really interesting, jumping to a "hook tree" (I am not sure about a graph as you have to consider cycles, etc.) instead of a "hook chain" adds a level of genericity. However I would not like to loose the ease of use of hooks with types: they are basically simple functions and the hook chain function composition.

For now the "chain" or the "pipeline" is the best suited use case in Feathers because hooks are executed in the request/response process of HTTP, eg what does mean a hook tree generating multiple outputs in this case ? But maybe what we are talking about here is to build a "function composition module" able to manage more complex chains and that will be used by Feathers for hooks, in that case it might be out of the scope of Feathers actually :-(

j2L4e commented 6 years ago

How bout providing a way to add metadata to hook functions? That way some stuff could be evaluated at chain construction time rather than everytime the hook is run. E.g:

// utility function
function hookWithMetadata(hook, metadata) {
  hook.metadata = metadata;

  return hook;
}

export function myHook() {
  const hook = ctx =>  {
    console.log(`I'm a useless hook!`);
  };

  return hookWithMetadata(hook, {
    sync: true
  });
}

This hook function is marked as sync so rather than checking for a promise everytime the function is called, the hook chain can be built with a sync function call. This is just from the top of my head, I'm sure that there would be other (better) use cases for hook metadata.

eddyystop commented 6 years ago

Two existing patterns seem not to be well known.

  1. The combine hook allows sequential execution of a set of hooks, e.g. combine(hook1(), hook2())(context). This allows you to write complex custom hooks using JS rather than creating a DSL for hooks.

This may be similar to @StevenBlack 's anchors.

  1. Workflows
    const blogs = [hook1(), hook2(), hook3()];
    const meetups = [hook11(), hook12()];
    ...
    before: {
    all: [ ...blogs, hook21(), ...meetups]
    }

Its easy to write a utility which creates HOC of hooks. I've written one for GraphQL resolvers for example. However workflows do much of the same, and it can be difficult to understand what a HOC structure does while David's hook design is clear.

eddyystop commented 6 years ago

We can write a utility function which returns a context or a promise. A rough idea (need to handle an array of course)

function syncAsync(result, context, func) {
  return typeof result === 'object' && typeof result.then === 'function')  ?
   result.then(func(context))) : func(context)(result);
  }
}

Wouldn't it be clearer and more scalable to call such functions inline rather than through attributes?

j2L4e commented 6 years ago

I'm aware it's not the best example, because sync/async is detectable at runtiem with negligible performance impact. Just food for thought.

claustres commented 6 years ago

These patterns are useful and easily implemented with current Feathers, eg in our krawler tool we apply some metadata allowing to apply a hook only when hook data match some criteria and a parallel hook that simply run a Promise.all() on others hooks. With these generic patterns you almost do whatever workflow you want.

However this does not fundamentally answer the question about controlling the hook execution order and how you can split your hook workflow easily in a modular approach for complex or customizable apps.

StevenBlack commented 6 years ago

Luc @claustres, in my abstractions, an Anchor is a subclass of Hook (because an anchor "isa" hook).

Since Anchor is a Hook, it can chain itself with other Hooks, including other Anchors. An Anchor is a conventional Hook in every way.

Except Anchors are super hooks because Anchors possess the ability to:

In this way you build a directed graph of hooks. The graph can be as complex as necessary. Typically a Hook is trivial; it does just one thing well. All the coordination logic gets coded into Anchors.

claustres commented 6 years ago

Yes your anchor covers it, I was talking about the discussion between @eddyystop and @j2L4e

eddyystop commented 6 years ago

@claustres These are my take-aways from glancing at krawler

  1. myHook(...) can include an options param with standardized properties. krawler has match which looks for a match with the hook data, and faultTolerant which swallows errors.

  2. A parallel hook which runs in parallel a sequence of hooks. Similar to runParallel + combine.

Am I missing anything? Anything else appropriate for a more general case?

@j2L4e Your metadata to hooks and the options idea above may be related.

claustres commented 6 years ago

You are almost right, in krawler:

We do not recurse but it could be possible, i.e. one hook run in parallel could also be a parallel hook...