search

mongoosejs / kareem

Next-generation take on pre/post function hooks
Apache License 2.0
82 stars 8 forks source link

grappling-hook #3

Open creynders opened 9 years ago

creynders commented 9 years ago

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 :smile: ) 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 commented 9 years ago

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 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 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 commented 9 years ago

Unless I misunderstood you 1) is already possible with context. And 3) is top of my wish list :smile:, 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 commented 9 years ago

@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 commented 9 years ago

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)

vkarpov15 commented 9 years ago

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 commented 9 years ago

@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!