Discussion: Autogenerating hooks

[ekryski]

Soliciting some feedback for where hooks should go. Currently the way I'm generating them:

server
  -> hooks
    -> <service>
      -> <before or after>
        -> <method>
          - <hook name>

So a realistic example looks like this:

server/
  -> hooks/
    index.js
    -> user/
      index.js
      -> before/
        -> create/
          remove-id.js

If you have a method that applies to all hooks for a service this would be fine. Here are some of my ideas.

Idea 1 (current implementation): deeply nested with an "all" group

server/
  -> hooks/
    index.js
    -> user/
      index.js
      -> before/
        -> all/
          remove-id.js
        -> create/
          some-other-hook.js

Idea 2: deeply nested, hooks that apply to all are just top level

server/
  -> hooks/
    index.js
    -> user/
      index.js
      -> before/
          remove-id.js
          -> create/
            some-other-hook.js

How option 1 would look in code

// Inside hooks/index.js
import user from './user';

export default function() {
  const app = this;

  return {
    user
  }
}

// Inside hooks/user/index.js
import removeId from './before/create/remove-id';

export default {
  before: {
    create: {
      removeId
    }
  }
}

// Inside services/user.js
import hooks from '../hooks';
import service from 'feathers-mongoose';
import User from '../models/user';

export default function(){
  const app = this;

  let options = {
    Model: User,
    paginate: {
      default: 5,
      max: 25
    }
  };

  app.use('/v1/users', service(options));

  app.service('/v1/users').before({
    create: hooks(app).user.before.create.removeId
  });
}

Advantage

It makes it very easy to track down where the hooks are. When building apps with lots of hooks it can be hard to keep track of them without an organized folder structure, especially if hooks in different services have the same name.

I think we want to encourage and make it as easy as possible to create a bunch of small hooks that do one thing well and chain them, rather than a few larger hooks that do a bunch of things.

Disadvantage

More boilerplate and scaffolding this out programatically from the generator is pretty complex.
The folder structure is easy, it's the importing of the hook, the generating of the index.js file if it doesn't exist, and adding the path of a new hook to an existing index.js file that is complex and could be error prone.

[ekryski]

There are two other options...

Idea 3: Flat but logically named

server/
  -> hooks/
    index.js
    before-user-all-remove-id.js
    before-user-create-some-other-hook.js
    after-user-find-remove-password.js

Idea 4: Flat and just the hook name

server/
  -> hooks/
    index.js
    remove-id.js
    some-other-hook.js
    remove-password.js

[corymsmith]

While #1 is a bit more verbose re: all folder I think it's more clear then just being in the root folder. I like the way it's all separated out. Easier to see what's going on vs just the flat structure

[jamesjnadeau]

👍

[marshallswain]

Ideally, we want a solution that

• is simple to use for the developer.
• accounts for highly-reusable hooks across services.
• accounts for service-specific hooks.
• is easy to generate.
• is easy to plug a GUI tool into later.

Since hooks can be highly reusable, we definitely need a general hooks library, but we also need a place to store hooks that are service specific. The only separation of hooks that makes sense to me is /before and /after, because hooks can be useful across multiple methods. So, to me, it doesn't make sense to use folders for the methods like /get and /patch unless we are going to number the files inside and have that be the hook execution order (which would result in a lot of copies of the same hooks, so it doesn't account for highly-reusable hooks).

I think it makes sense to put each service in its own folder and include anything specific to the service in that folder, including the ORM model. More like this layout:

I can see it being a fairly common scenario of starting out an app with multiple services in one server , then wanting to roll off a high-traffic service to its own machine/server. I think that this layout would facilitate that a little bit.

Here's an example service's index.js:

// index.js
import mongoose from 'mongoose';
import service from 'feathers-mongoose';
import User from './model';
import before from './before';
import after from './after';

mongoose.Promise = global.Promise;

export default function(){
  const app = this;

  mongoose.connect(app.get('mongodb'));

  let options = {
    Model: User,
    paginate: {
      default: 5,
      max: 25
    }
  };

  app.use('/v1/users', service(options));

  // const service = this.service('v1/users');

  /* * * Before hooks * * */
  service.before(before);

  /* * * After hooks * * */
  service.after(after);

  /* * * Set up event filters * * */
  service.created = service.updated = service.patched = service.removed = events.requireAuthForPrivate;
}

Each service could have a before.js and after.js with the hook configuration object in it. Having it by itself would keep it simple for a developer, but also make it easy to use with a GUI builder tool. Each service would also have its own hooks folder, so you'd end up with something along these lines in the config:

// Bring in the global (highly-reusable) hooks.
import hooks from '../../hooks';

// Bring in hooks from the auth plugin.
import {hooks as authHooks} from 'feathers-authentication';

// Bring in service-specific hooks
import userHooks from './hooks';

export default before = {
    all: [],
    find: [],
  get: [
    authHooks.setUserID(), 
    userHooks.doSomething()
  ],
  update: [],
  patch: [],
  delete: [
    hooks.preventUserFromDeletingSelf()
  ]
};

In this scenario, I would like to see the <service>/hooks/index.js file be a dictionary of all other hooks in the folder. We could even do a naming convention so that a file with the name my-groovy-hook.js would become available automatically at serviceHooks.myGroovyHook. Those who wouldn't want to use the convention could simply delete the index.js file and import hooks however they wish in the hook config.

[daffl]

Some notes from me:

1. I'm not sure a separation between before and after is necessary. Some could technically be used for both and it should be fairly clear by their functionality when they are supposed to happen (e.g. where things like remove-password would be). The first issue submitted for hooks was normalize parameters feathers-hooks#1 suggesting to normalize the hook object so they basically look the same except for the result property and the type.

2. I agree with everything else @marshallswain said. Separating between general and service specific hooks will probably be important. It will be a little trickier for the generator because you'll have to create a list of all services (recursively?) but making it easier to move a service with everything that belongs to it is a very good point.

[marshallswain]

I actually thought that same thing while I was writing the above. I'm not sure the separation is necessary. I've not encountered a case where I needed one of my after-hooks as a before- or vice versa. I tend to put the before. and after. prefix as a simple reminder of where it's being used, but it still doesn't prevent me from having to open the hook config, so I guess there really isn't a strong argument for doing even that.

I'm still trying to figure out a nice way to show the hooks interface in the GUI. It would be nice for the user to be able to browse the installed hooks, as well as other community-shared hooks. We would need some sort of tagging system, or self-documenting system similar to what node machines does, in order for the library of hooks to be really accessible. That way we can add tags for before and after, the CRUD methods that apply, and whatever other arbitrary tag, like auth. Simply using npm as the tagging system won't work as well because you can have multiple hooks per npm module, so we'd need a common format that can be easily parsed.

[marshallswain]

There are basically four places that I can think of where we'll be defining hooks.

• npm modules dedicated to hooks (like with import {hooks} from feathers-commons)
• the server/hooks folder for shared hooks.
• the <service>/hooks folder.
• other plugins like feathers-authentication

We are probably going to create a feathers-common-hooks module, or maybe just put them in feathers-commons. Maybe we can have the generator import the feathers-commons hooks and set them up in the server/hooks/index.js, so they're ready to be used right away. Maybe that's the way to handle the auth hooks, too? That would basically narrow it down to only two places where we have hooks defined. The server/hooks folder and the <service>/hooks folder.

[marshallswain]

I'm picturing server/hooks/index.js looking more like this:

// jshint unused:false
import {hooks as auth} from 'feathers-authentication';
import {hooks as common} from 'feathers-commons';

let hooks = {
    auth,
    common
};

export default hooks;

And maybe we could do the file system convention here as well. (where my-hook.js is set up on hooks.myHook)

Then we would remove the line that says .configure(myHooks) from the app.js in the generated app.

[marshallswain]

I'm not sure how to dynamically import modules in es6, since import statements have to be on the top level. Here's a quick module to handle importing the directory.

// Read all hooks from the file system and arrange them into a single object.
import fs from 'fs';

let serviceHooks = {};

fs.readdir(__dirname, (err, files) => {
  files.forEach(fileName => {
    if (fileName.indexOf('.js') >= 0 && fileName !== 'index.js') {
      fileName = fileName.replace('.js', '');
      let camelCased = fileName.replace(/-([a-z])/g, function (g) { return g[1].toUpperCase(); });
      let module = require('./' + fileName);
      module = module.default ? module.default : module;
      serviceHooks[camelCased] = module;
    }
  });
});

export default serviceHooks;

It supports either a default export or multiple keyed exports, but not both.

[daffl]

Honestly, I'd rather have it imported explicitly. My experience was that those magical folder importing index files are always prone to errors (e.g. you can have really weird errors if you happen to put a file in that exports a function but not like we expect). I have a template to write the imports into the file I'll just have to finish that up this weekend.

[marshallswain]

Honestly, I'd rather have it imported explicitly.

That works.

you can have really weird errors if you happen to put a file in that exports a function but not like we expect

This is where an official hook spec would come in handy. We define the expectations.

[daffl]

Yes but I mean e.g. someone accidentally (or purposely) puts a non-hook into the folder that happens to return a function. Also other questions like how do we handled nested folders etc.?

[marshallswain]

Hah! from the code you can see that I was not even thinking about folders.

[marshallswain]

Based on what we've talked about here, I'm closing this PR: https://github.com/feathersjs/generator-feathers/pull/15/files

[ekryski]

Ya @daffl and I discussed this same thing of automagically importing hooks last week. I should have included you in that discussion @marshallswain. We should be more cognoscente of putting all that stuff in either public domain or the core group in slack.

I'll wrap the generator changes we have been talking about today and then I'm going to be working on the chat example/documenting it.

[daffl]

If you are working on the generator, don't forget about #14 it has some code for adding the imports to the index file but it doesn't work yet. Still good to merge though I'd think.

[ekryski]

Yup sweet! Merging right meow. 😽