Skip to content
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

grappling-hook #3

Open
creynders opened this issue Apr 27, 2015 · 6 comments
Open

grappling-hook #3

creynders opened this issue Apr 27, 2015 · 6 comments

Comments

@creynders
Copy link

Hi @vkarpov15

First a little background before I come to my question. I'm a core team member of keystone and recently we needed some hooking functionality. So I started scouring over the many hooking modules (hooks, hooks-fixed, kareem, etc) and thought kareem definitely looked the most promising. However, we also needed a bunch of things that weren't provided out-of-the-box so at first I tried to adapt kareem to our needs. As it always goes, I ended up writing my own hooking module from scratch, thinking it would be faster to write one than adapting yours (it probably wasn't 😄 )
Anyway, this became grappling-hook. Due to the fact we use and expose mongoose in keystone I really wanted to make the pre and post registration API compatible to mongoose's, in order to present users with a single hooking API, whether they're hooking into mongoose or keystone. It's not 100% compatible ATM, but will be pretty trivial to do so.
Anyway, I can imagine you have a LOT of work with mongoose and was wondering if you'd be interested in considering using grappling-hook in mongoose. I had a superficial look at mongoose and it seems to me it would cost me very little effort to make grappling-hook a drop-in replacement for both kareem and hooks-fixed, but providing a bunch of extra functionality out-of-the-box, e.g. parameter passing to pre middleware, more granular control over post sequencing, a number of sugary methods which allow you to introspect middleware et cetera.
Granted, it was a superficial look and I'd definitely need to compare performance with kareem. In our case performance is not such an issue, so I definitely haven't optimised the hell out of it.
Just giving you a heads-up that I wrote something similar, and will actively maintain it, with promises integration on the roadmap for instance.

Please do not misunderstand me, I'm not suggesting this because kareem is badly written or so AT ALL (again: AT ALL). My main motivation is that I really want to help make keystone the best it can become and to us it would be really cool to be 100% sure the hooking mechanism in keystone and mongoose is identical. And the benefit I think it could have for you is being freed of a small yet essential part of functionality of mongoose.
If it doesn't interest you: I TOTALLY understand, if so, sorry for bothering you with this silliness.

@vkarpov15
Copy link
Member

Worth noting - I consider hooks and hooks-fixed to be deprecated. hooks-fixed only exists because I don't have push access to hooks and hooks has several critical bugs. grappling-hook is worth investigating, I like the idea of collaborating on code where possible, but there are 3 features / things that I really need from a hooks library going forward -

  1. Executing hooks attached to a different object. In mongoose we need to recreate hooks every time we create a new doc or subdoc, which is pretty wasteful (see performance issue of 4.0.0&4.0.1 Automattic/mongoose#2809). Much of the point of kareem was "attach hooks once to schema, call them from wherever" as opposed to "execute complex logic to add hooks to prototype every single time a doc is created." I may be able to implement this with callHook's context param but I want to be sure.
  2. 100% test coverage. It's not a guarantee of being bug-free, but it's a good start.
  3. Promises support. See Refactor $__registerHooksFromSchema Automattic/mongoose#2754. Mongoose does a lot of nasty things to make hooks play nice with promises. Kareem doesn't have built-in support for promises, but being able to do execPre(), execPost(), and wrap() gives me the ability to just use mongoose's promises.

I'm curious to see if I can do 1) and 3) with grappling-hook. What do you think?

@creynders
Copy link
Author

Unless I misunderstood you 1) is already possible with context. And 3) is top of my wish list 😄, i.e. it's the next thing I'll be working on.
Agreed on 2) and I think there's still some edge cases to be investigated there. If you start warming up to the idea, I'll aim for getting grappling-hook pass all mongoose hook-related tests to start off.

@creynders
Copy link
Author

@vkarpov15 just a heads-up on the release of v3.0.0 of grappling-hook, which has full promise support. It's library-agnostic, i.e. you can use any Promises A+ compliant library with it.

There are some notable differences with how kareem does things: functions registered to post are executed before the final callback is called. E.g.

instance.pre('save', function(){
  console.log('pre:save');
});

instance.post('save', function(){
  console.log('post:save');
});

instance.save = function(cb){
  console.log('saving');
  cb();
};

instance.save(function(){
  console.log('final callback');
});
#output:
pre:save
saving
post:save
final callback

Would you consider having this kind of changes to mongoose pre/post? Or do you want it to be 100% backwards compatible?

@creynders
Copy link
Author

Sorry, forgot to mention the features: (terminology: hooks=the functions you register other functions to, middleware=the functions you register to hooks, i.e. in the above example 'save' is the hook and the functions registered to pre:save and post:save are middleware)

  • sync hooks
  • async hooks
  • thenable hooks (i.e. hooks that return a promise)
  • sync middleware
  • async serial middleware
  • async parallel middleware
  • thenable middleware (i.e. promise returning functions)
  • custom qualifiers (i.e. you can use something else than pre/post)
  • presets (i.e. configure once, reuse everywhere)
  • wrapping of existing methods
  • direct calling of hooks
  • executing middleware in a different context
  • optimised enforced synchronicity, i.e. asynchronous/thenable hooks always finish asynchronously, but as fast as possible
  • full error consistency: synchronous hooks bubble, asynchronous hooks pass thrown errors to the callback's err object and thenable hooks catch all errors (thrown or otherwise) and call the rejectedHandler of the final promise.
  • and some extra sugar for introspection and leeway for special cases
  • 100% test coverage

@vkarpov15
Copy link
Member

Kareem actually does the same thing re: post hooks being executed before the final callback. Mongoose really should be doing that going forward.

Grappling hook is looking pretty sweet, I'm gonna take a look at it when I get a chance. Thanks for the great work :)

@creynders
Copy link
Author

@vkarpov15 ah, now I see, you're using both hooks-fixed and kareem in mongoose.
If there's anything I can help out with, let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants