What with results of wrapped methods?

[creynders]

ATM all middleware gets the original arguments passed as parameters and are executed in the scope of the emitting instance, e.g.

instance.whatever = 'mofo!';
instance.pre('save', function(a,b){
  console.log(a); //outputs 'foo'
  console.log(b); //outputs 'bar'
  console.log(this.whatever); //outputs 'mofo!'
});
instance.save('foo', 'bar', _.noop);

However, no doubt there's situations where we want to pass some kind of result to post middleware. Question is: how? Would it make more sense to not pass the original arguments to post middleware, but the results instead? Or should we pass it as the first parameter and shift the others? E.g.

instance.post('save', function(result, a,b){
  console.log(result); // outputs the results of the `save` operation
  console.log(a); //outputs 'foo'
  console.log(b); //outputs 'bar'
});
instance.save('foo', 'bar', _.noop);

@JedWatson ?

[JedWatson]

@creynders sorry for the delay - didn't notice the notification for this. Is it still a thing?

If so, think your suggestion about passing results as the first parameter makes sense. Initially, I was inclined not to shift the others, and add results to the end, but then you could have a function that accepts a variable number of arguments, and confusion would ensue. So first + shift seems better.

[mattapperson]

@JedWatson @creynders I agree and think this is important. I also think middleware should be able to augment the results and see the augmented results from other middleware (though I don't think you need to know who did it except as maybe part of a debug mode?)

[mattapperson]

I would motion that async.waterfall is used vs async.each to accomplish this

[creynders]

Yeah, I think there's a use case for passing the results to post middleware. However, this should definitely not be the default. Question is: how should the enabling look like? Do we want this strictly in the producer API, or do we want to allow the consumer to opt-in/out as well? And if so, how granular should this be? Method-per-method? Instance-per-instance? Or both?

@mattapperson do you suggest to allow middleware to change the signature of middleware functions down-stream? Cause that's what happens in async.waterfall. TBH you'll have a very hard time convincing me of the benefits, since that's a world of pain you'll be subjecting yourself and the consumers of your hooks to.

[creynders]

To elaborate a little: once you allow middleware to manipulate which parameters are passed on to the next middleware you'll enter the twilight zone. The problem is that it's an implicit dynamic contract, you have to know which middleware is hooking up where and in which order, if any manipulative middleware gets added in between all/some subsequent middleware will start failing. This might or might not happen loudly, but it's the subtle ones that bite you in the ass. Things will start failing, maybe even irregularly to make matters worse, and you'll have no idea why and where. It's a guaranteed fuck-this-shit-scenario.

However I'm working on a DI-lib, which could come to the rescue, if this is something you @mattapperson and others really want. It would allow you to make the contract explicit, and let the middleware decide which parameters they need, i.e. it would allow for parameter sifting. There's two things that need to happen first though, the DI-lib needs to stabilise its API to :shipit: and I have to find out how far it will integrate into keystone.

[mattapperson]

@creynders no, I am saying support 1 argument. Just because waterfall supports passing more does not mean we have to pay attention to more. Any method in js can have extra Params sent to it.

[creynders]

@mattapperson but that's already the case, the hook provider simply needs to provide it explicitly:

instance.pre('whatever', function(accumulator, next){
  accumulator.foo = { name: 'foo' };
  next();
});
instance.pre('whatever', function(accumulator, next){
  accumulator.foo.baz = { name: 'baz' };
  next();
});

var acc = {};
instance.callHook('pre:whatever', acc, function(accumulator){
  console.log(accumulator === acc); //outputs: true
  console.log(accumulator.foo.baz); //outputs: {name: 'baz'}
});

[mattapperson]

I'm saying:

instance.pre('post-request', function(origRequest, accumulator, next) {
    accumulator.body.foo = 'bar';
    next(null, accumulator);
});

instance.pre('post-request', function(origRequest, accumulator, next) {
    accumulator.body.bar = 'Foo';
    next(null, accumulator);
});

instance.callHook'pre:post-request', function(accumulator, next) {
    console.log(accumulator.body);
// output: {foo: 'bar', bar: 'foo'}
});

[creynders]

There's a number of problems with that approach:

1. grappling-hook isn't meant for handling requests specifically
2. even if it were I have no way of knowing what origRequest or accumulator is supposed to be, except for accumulator I could create a vanilla object obviously
3. if choosing the latter, then we'd need to allow API creators to override the creation of such a vanilla object and inject their own accumulator object
4. this would totally break all backwards-compatibility
5. it would seriously limit flexibility
6. IMO it's the responsibility of the API creator to determine what should be passed to the middleware and what not, except maybe for the results of the wrapped method to post middleware as described up top.

Due to all of the above reasons I think it would mean a lot of complexity for very little gain and the use case far too narrow to be considered in a generic library like this.

[mattapperson]

@creynders if I may, let me provide my feedback to your points.

1. this was an abstract example so please don't take it to literally. 😊 I think there are many more use-cases for this though.
2. in my mind origRequest in my example is just the default of whatever is being passed to hooks. accumulator would at first be the exact same. Hooks would augment this and keep passing it on. origRequest would then just be a reference for knowing what the orig request looked like. For best results, origRequest would be frozen / immutable.
3. See point #2
4. Perhaps.It's a consideration point.
5. A statement of perception I suppose. I think it adds a great deal of flexibility
6. This is true, however my point being that the hooked code needs to be able to access what middleware does, not just the inverse.

To elaborate on point 6, right now "middleware" is little more then event listeners (correct me if I am wrong) and has no ability to effect the greater system other then APIs provided. Now I can only assume you feel that to be a correct course given your above comments.

To provide another example where this is important:

1. In wordpress, every time anything requests a list of posts, a hook is called (grappling-hook / keystone can do this too)
2. In wordpress In my hook that gets called when posts are called for I can not only know that posts are being loaded and do something with them, but I can know who called them and filter the results to remove any posts with the string [Beta] in the title and pass those filtered results back to wordpress. In this way, no part of my wordpress site would ever see posts with [Beta] in the title. (grappling-hook / keystone currently can not do this afaik)

Sorry to be so persistent about this, if after this post you still disagree I will drop it I promise 😊

[mattapperson]

Note: To me / for me this ties in heavily to https://github.com/keystonejs/grappling-hook/issues/14

[creynders]

@mattapperson no problem with being persistent, it's through discussions like these I've learned a LOT, not just technical stuff, but radically different points-of-view, which is always enlightening.

2/ that's definitely out-of-scope. Cloning stuff (and making it immutable) is first of all hard, but also potentially very cpu-straining and memory consumptive, again that's definitely a responsibility of the API creator, i.e. the hook provider.

6/ and you're right, grappling-hook is little more than a two-phased pubsub system. But, that's the whole point: it should not have an effect on the greater system. It's an enabler nothing more. In WP the hooking system is baked in, we preferred to extract and abstract it, that's all.

However, grappling-hook does not forbid what you aim to achieve in any way, that's largely how routing middleware functions: e.g. process, process, filter, transpose, process some more, pull in extra data, process and finally send it out. It just needs to be set up at the correct place, which would be keystone in this case, not grappling-hook.

[mattapperson]

@creynders fair enough. I only came here because @JedWatson recommended that this was the right place for it when I was talking to him 😊

[creynders]

@mattapperson yeah, on slack I assumed you meant to allow middleware to manipulate which parameters are being passed through, which discussion would belong here. Sorry for the confusion/sidetrack.