Swap prepost for grappling-hook

[creynders]

With grappling-hook having its pre/post middleware registration rewritten, its fully keystone 0.3.x compatible. This is a replacement for #1239 and uses the new grappling-hook version.

Let's finally

I'll make a separate PR for adding a few oft requested hooks.

[dcousens]

Untested ACK from me. Will test tomorrow.
LGTM though.

[creynders]

@dcousens The (idea of the) last commit was cleared with @JedWatson BTW on slack. A bit too fragmented to paste here.

[creynders]

We’ve got a bit of an ambiguous situation BTW: Currently in keystone pre:routes middleware is considered to be express middleware, however it is registered with our pre/post methods, so the question is: do we want to keep them as express middleware or are we going to regard them as grappling-hook middleware that happens to be used as express middleware?

creynders [14:17]
Code-wise, both are trivial to setup with grappling-hook.

creynders [14:18]
In concreto: ATM all pre:routes middleware is fed directly to app.use, i.e. they are all separate express middleware functions

creynders [14:19]
while in the other case I’d set up a single express middleware function which calls callHook(‘pre:routes’)

Benefit of going for grappling-hook middleware: it’ll allow all grappling-hook kind of middleware:
function(req, res) //sync function(req, res, next) //async function(req, res, next, done) //async paralell (edited)

creynders [14:22]
And is obviously more consistent with the other hooks we are/will be providing

creynders [14:23]
And it’s still express middleware compatible, i.e. people won’t know the difference

jed [14:23]
I like it

[JedWatson]

@creynders don't we want grappling-hook ^2.1.0 here?

Super uneasy about the lack of tests over those file field types. Let's fix that soon 😣

[creynders]

Ah, yes, nice catch @JedWatson

force pushing it in a sec

[dcousens]

Can we get this in a keystone branch so that we can work in this PR
together?
On 27 Apr 2015 10:39 pm, "creynders" [email protected] wrote:

Ah, yes, nice catch @JedWatson https://github.com/jedwatson

force pushing it in a sec

—
Reply to this email directly or view it on GitHub
keystonejs/keystone#1346 (comment).

[creynders]

Sure... but why exactly @dcousens ? Wouldn't it better to land this and then have separate feature branches for adding more hooks?

[dcousens]

I was going to help put this under test.
On 27 Apr 2015 22:59, "creynders" [email protected] wrote:

Sure... but why exactly @dcousens https://github.com/dcousens ?
Wouldn't it better to land this and then have separate feature branches for
adding more hooks?

—
Reply to this email directly or view it on GitHub
keystonejs/keystone#1346 (comment).

[dcousens]

I'm happy to submit the PR to you, just figured I'd ask.
On 27 Apr 2015 23:01, "Daniel Cousens" [email protected] wrote:

I was going to help put this under test.
On 27 Apr 2015 22:59, "creynders" [email protected] wrote:

Sure... but why exactly @dcousens https://github.com/dcousens ?
Wouldn't it better to land this and then have separate feature branches for
adding more hooks?

—
Reply to this email directly or view it on GitHub
keystonejs/keystone#1346 (comment).

[creynders]

@dcousens Ah no sorry, thought you had a different reason altogether. I'll close this one, and create a new PR from a keystonejs branch

[JedWatson]

I think it's fine to merge this and add tests in a separate PR. From my review, it looks good so I'm happy to have it in master. We've also got a stable build out there as of the weekend so it's a safe time to do that. We'll work through any issues that come up.

[creynders]

🌈 🐧 🌷