Wrap each middleware in array with provided middleware

[fl0w]

I'm just getting the ball rolling here.
All tests are passing, though no new tests are written yet.

Probing (no pun intended) to check if this is the direction koajs members want to take this.

Related discussion/issue: koajs/koa#219

[jonathanong]

hah shoot. this is so simple. maybe too simple. 👍

/cc @koajs/logging @koajs @PlasmaPower @jeromew

[fl0w]

@jonathanong I say, why not leave the complexity to a possible koajs/profiler?
Or, koajs/timestamper á la:

function(fn) {
  return (ctx, next) => {
    const start = process.hrtime()[1]
    console.log(' > ' + start)
    return Promise.resolve(fn(ctx, next)).then(() => {
      const end = process.hrtime()[1] - start
      console.log(' < ' + end + ' (' + end/1000000 + ')')
    })    
  }
}

Edit removed fn.name for obvious fat-arrow reasons.

[fl0w]

@PlasmaPower also raised concern regarding if wrapper should be an Array or simply a fn.

[dead-horse]

LGTM

[PlasmaPower]

This might be a bit crazy, but what if we had ctx.wrappers as an array? This would allow compose, mount, router, etc to work without another wrapper argument. While mount would need to add support for this in, router should work perfectly with wrappers given that wrappers would be sent as part of the context (which is preserved) not arguments (which wouldn't be preserved down the compose chain). In addition, you could use a profiler just like you'd use middleware.

app.use(static('/')); // This wouldn't be profiled
app.use(profiler()); // Attach the profiler
app.use(router.routes()); // This would be profiled
app.use(mount('/other', static('/other'))); // Both mount and static would be profiled

The profiler could even be mounted to routes. It could observe the overhead of mount or koa-router, and if it's really smart, it could generate a graph of middleware and the overhead of each.

Another use of this is if you wanted to, for example, use your own type of middleware (we have generators, promises, who knows what else might come up). If we create wrappers as an array, it could coexist with a profiler. It would have to come before the profiler though. We could mitigate that if we wanted with multiple stages, but I don't think it would be worth it.

The one disadvantage of this is that we have to wrap middleware at "runtime" (or "requesttime" :P). However, given that this PR is already doing that and nobody mentioned anything, I'd say it's not a problem (yes I know it's a proof of concept but still). In addition, wrappers shouldn't take too long to do the wrapping, the added cost of running the middleware will be the main performance cost. If we do merge this PR as-is though, I'd prefer if we wrapped the middleware before hand.

[jeromew]

@PlasmaPower'PR is for koa2 and I agree that it is now implemented as a runtime wrap ; it could be modified with middlewares wrapped earlier as I see. Only a perf test could show the impact of this.

Regarding koa1, the impact of a runtime wrapping would be more important I think (but I have no tests to prove anything) but I like the idea of targeting some requests only. Sometimes you simply want to profile a sample of requests, from start to finish. Reserving a property on context for profiling could help for creating targeted profiling (on a request or on parts of the flow).

As for a wrapper versus an array of wrapper, is there a difference between

compose(mws, [wrap1, wrap2])
and
compose(mws, compose([wrap1, wrap2]))

the array could be interesting if there are reasons to maybe modify the array on the fly but I have no experience with the problematic of a stack of wrappers.

Maybe this is also linked to the need for introspection in the middleware graph. Once an array of middleware is composed, there is no way to know which middlewares it was composed of.

Also, I don't know where we are on mw naming, but I think that a wrapper/profiler should have a clean access to the name of the mw, be it fn.name or fn._name, because middleware graphs can become complicated and profiling a string of anonymous mw can be tiring.

[PlasmaPower]

@jeromew From what I understand, there's a good chance we'll be moving to v2 as default soon. As for arrays, using compose doesn't make any sense, as the wrappers aren't middleware. You also can't do wrap1(wrap2). You'd have to do something like this:

compose([a, b], fn => wrap1(wrap2(fn)));

I think an array would be cleaner. Also, if we're storing it in ctx, then an array would be much cleaner.

As for naming, the standard is mw._name || mw.name. That standard would work fine with a wrapper function, which would be passed the middleware function (which stores it's name).

[fl0w]

As far as targeting middleware for wrapping, there's nothing stopping user from composing and passing a profiler themselves, i.e.

const compose = require('compose')
app.use(someMiddleware) // not profiled
app.use(compose([a, b, ...], profiler)) // profiled
app.use(someOtherMiddleware) // not profiled

[PlasmaPower]

@fl0w Yeah I agree that the targeting of middleware isn't that big of a deal. The main reason I'd like ctx.wrapper is so that things like router, mount, and compose can all work without an additional argument, and so that the profiler can generate a graph of middleware.

Edit: one thing that solution doesn't address is only profiling /api or something. It could be implemented with a wrapper checking the url though. However, the middleware solution is how we've been doing url-dependent things. It's also more versatile.

[fl0w]

@PlasmaPower I'm not sure I understand when ctx.wrappers actually would wrap? This would be a koajs/koa feature rather then a koajs/compose?

The simplicity behind this is that it delegates to user space. I like that, some might want to extend an emitter, or use a "singleton object" for storing additional isolated metric data (which neither context/koa or application cares about).

Edit; And if need be, a user can create a stack of functions - as long as the returned root fn is of v2 signature.

[PlasmaPower]

@fl0w it would be implemented in compose and mount, but router wouldn't have to implement it as it uses compose (whereas with the current PR it would). It's still delegated to userspace, but koa would have to/should make a wrapper array in the Context object (but never touch it).

[jeromew]

@PlasmaPower ok I didn't know that koa2 was so close. btw I was away for quite some time, is there a writeup somewhere that I can read about the rationale of the change from generators to promises ?

to me a wrapper is nearly a middleware when it can do something before the mw on the way downstream and after the mw on the way upstream. Is this wrong ?

[fl0w]

@jeromew To be clear; I'm the new guy. This was taken from koajs/koa#219.

[PlasmaPower]

@jeromew You can find some discussion of koa v2 at koajs/koa#533.

Profiler wrappers would usually work as if middleware was added before and after every middleware, that's why they are wrappers, they wrap the middleware. However, wrappers could also do things like convert types of middleware to promise based middleware.

[PlasmaPower]

If we implement this, especially if we implement this as an array, I'd be in favor of removing the deprecated generator to promise conversion in favor of koa-convert providing a wrapper to convert generators to promise-based middleware. I think that would be the more versatile approach, as some people might want to use generators with the new argument syntax until async/await arrives. The current approach would convert them as if they took ctx as this instead of an argument, and you'd have to extend the Koa Application to change that.

[fl0w]

@PlasmaPower Unless you want to create a PR, I can update PR later today according to your suggestions (as I understand them) later today (CET TZ).

Effectively it comes down to

• Grabbing from ctx.wrappers, if any, instead of passing to compose.
• running "current" middleware[i] through all ctx.wrappers.

Does this reflect your thoughts?

I like that wrappers becomes multi-purpose; It's almost as if they're mw-decorators.

[PlasmaPower]

@fl0w I'll create a separate PR. I wanted a bit of feedback first.

[fl0w]

I actually prefer this approach (to #52), even if it's more invasive with an added argument. I feel #52 is too opinionated, IMHO.

[PlasmaPower]

Yeah #52 is opinionated. However, I also think that it leads to the best experience when using wrappers. IDK.

[jonathanong]

haha why not just

let fn = middleware[i] || next
if (wrapper) fn = wrapper(fn)

[PlasmaPower]

We don't want to wrap next.

[jonathanong]

why not?

[PlasmaPower]

next is guaranteed to be a function in conformance with standard Koa practice. It's not coming from the user 90% of the time. If you use co.wrap as a wrapper, you don't want it processing next.

[PlasmaPower]

Oh, also, a profiler shouldn't run on the next function, that just wouldn't make sense.

[jonathanong]

LGTM too. only features it's missing that should probably be in the framework is:

• we should be able to wrap multiple functions
• we should be able to enable/disable the hook

do you guys think we should do that here or in the framework?

[PlasmaPower]

Well we need tests of course. I'd prefer a wrappers array as previously mentioned.

we should be able to enable/disable the hook

How/why? I'm not sure I get your point there.

[jonathanong]

many times it's just for debugging. you might want to disable it in production. for example, you can enable/disable debugs at will. we're not going to do it at the middleware level.

of course, you could always do something like:

const wrappers = process.env.NODE_ENV === 'production' ? (fn) => {} : null

but then that might get annoying. it's up to you guys on how you think the best dev UX is.

the biggest problem IMO is that people are probably going to nest compose(), so it'll be hard to make sure the wrapper is used in every middleware. maybe we should flatten() the middleware

[fl0w]

@PlasmaPower compose would get the wrapper from app.wrapper. But I agree, there's no real difference between this and koa/Application.callback = () => compose(this.mw.map(wrapper)), which would be a Koa, not compose, -thing. Same issues apply though, whenever a library composes on their own they need to follow convention or wrappers become unintuitive/gimmicky.

Any other crazy ideas to approach this differently? Have all mw in compose be wrapped in an emitter and just emit pre/post? Force all lib authors to push mw onto app.middleware and have compose traverse a mw-tree once through Koa instead? lol.

Edit This was an issue in 2014 already #6 (comment)

[PlasmaPower]

@fl0w how would compose get the app object though? through the context? then why not skip the app altogether and just store the wrappers in the context?

The difference with changing Application.callback to wrap the middleware or having use wrap is that this would hopefully be supported by things like koa-router.

[jonathanong]

hmmm. i see pros and cons with all the implementations. but i think we all agree that the API for the wrapper should just be fn = wrap(fn).

how about we just create a module that creates a wrapper:

const wrap = new Wrapper({
  // default `true` always
  enabled: process.env.NODE_ENV === 'production',
})
wrap.push(fn => {
  return async (ctx, next) => {
    console.log('before')
    await fn(ctx, next)
    console.log('after')
  }
})

then using it must be done manually:

app.use(wrap(async (ctx, next) => {
  this.status = 204
}))

this is the least opinionated and the API isn't too bad (a lot of koa modules already wrap functions like this). routers like koa-router can simple have an api like router.wrapper = wrap or we can attach it like app.wrap = wrap and let routers access it.

regardless where wrap is actually stored (app, context, or compose), it will be used the same.

what do you guys think? if one of you wanna make it, we can add it to the org and basically make it official

[jonathanong]

just realized that Wrapper can't be a constructor because it returns a function, but you guys get it!

[PlasmaPower]

@jonathanong that could get messy, with some middleware having wrap called and others not because whatever uses them has wrapping builtin. However, if anyone wants to make that, here's a small thing to do it: (off the top of my head, might need troubleshooting)

module.exports = function (opts) {
  opts = opts || {};
  opts.enabled = opts.enabled || true;
  var ret = function (fn) {
    if (opts.enabled) {
      var wrapped = this.wrappers.reduce((mw, wrapper) => wrapper(mw), fn);
      wrapped._name = 'wrapped-' + fn.name;
      return wrapped;
    } else {
      return fn;
    }
  };
  ret.wrappers = [];
  return ret;
};

[PlasmaPower]

I'd prefer we have koa, router, etc accept a wrappers array though. We could still have that module as a backup I guess for when something doesn't support it. If you're wondering about enabling with that method, I'd do this:

if (process.env.NODE_ENV === 'development') {
  app.wrappers.push(profiler);
}
app.use(static('/'));
app.use(router({ wrappers: app.wrappers }));

[jonathanong]

so that's just basically app.wrappers = [] and app.use(router(app)), which i think is how the router APIs are already. i'm cool with that too

[PlasmaPower]

I feel like there should be a way for the router to get the app without being passed it, I mean it's right there in the same line of code. A _onUse hook might work (call mw._onUse(app) if it exists) but the problem is that it only applies to the first layer of middleware without middleware support. Any other ideas?

As for passing it app, at that point why not get the wrappers from ctx.app at runtime, and at that point why not my idea? I guess app.use(compose(middleware, app) seems more silly than app.use(router(app)) does, because right now compose doesn't care about the context.

[fl0w]

@PlasmaPower sorry, been AFK. I understand your point now. You're right, I couldn't create a wrapper-lib that grabs wrappers from context with this wrap(fn)-esc solution.

I dislike having to app.use(wrap(mw)) in my app though, because in this case; There's no need to implement wrappers into either Koa or compose. I can do that myself as it is right now.

[jonathanong]

we could also go the route of app.wrappers = [] and having users pushign to it.

then have koa-compose flatten() the array before running it. this way, all middleware will use the wrappers from the top-level app.

only problem with this is that routers will HAVE to return [middleware...] instead of returning a composed version, which could be a breaking change.

[jonathanong]

and if we memoize the flattened array, then users may not be able to edit the routing stack after app.callback(), whcih may be a feature or a bug

[PlasmaPower]

@jonathanong A router can't simply return a list of it's midlleware, they have to be wrapped by the router first, and at that point wrapping the wrapper is useless.

[jonathanong]

@PlasmaPower true, depends on the router. i guess they could always compose(middleware) again if they wanted to. routers like app-router are already passed the app instance so they should have app.wrappers

[PlasmaPower]

@jonathanong I'm pretty sure this would mean that both koa-route and koa-router (the two most popular routers) would need to be redesigned.

[jonathanong]

koa-route should be fine because it's one-route/one-method/one-middleware.

i just realized koa-router doesn't have the middleware = require('koa-router')(app) API anymore. maybe taht was a really old version. in that case, yeah, the user might have to pass .wrappers somehow

unless i'm missing osmething?

[PlasmaPower]

@jonathanong the problem is that koa-route wraps the middleware itself (source). Wrapping the wrapped middleware won't do anything, the middleware needs to be wrapped before it's wrapped by koa-route.

Edit: that's a lot of wrapped :P

[jonathanong]

oh i see. i didn't mind jsut wrapping the wrapped middleware.

but if that's the case, then we should have app.wrap(fn) or something to make it easier to wrap specific middleware.

[PlasmaPower]

@jonathanong say you want to use co.wrap as your wrapper - you couldn't do that with this system and koa-route. This would pretty much restrict it to a before and after hook.

I'm not sure what you mean by app.wrap(fn), would it be something like the call-middleware repo I made? If so, I'm not sure how much that would help because you'd need the app anyway. The only way to get that with the current koa-route API is through the context at runtime, and at that point we're opinionating it already so we might as well go with my solution.

I think another alternative is a .onUsed API, which could pass middleware a number of things, the obvious one being the app it's being used on. Things like routers and compose should call .onUsed with the app they got from their .onUsed.

[jonathanong]

yup. i think we have to put it in context now for all your use cases. we should've just had all those use-cases set in stone first :P and a test would be necessary for each.

so koa-compose will use call-middleware on every middleware and routers and such who wrap user middleware will use call-middleware themselves, right?

[PlasmaPower]

@jonathanong yep, anything that calls user middleware should use call-middleware. We could also put the function in app, but I thought a module would be a bit cleaner.

[jonathanong]

ok cool. i'm convinced. if u wanna put together all the use-cases and try to get a consensus, taht would be great :)

sorry, i've been busy and haven't really been lookinga t open source stuff lately.

[PlasmaPower]

@jonathanong no problem :). Another problem this solves is that I accidentally passed generator middleware to koa-mount, and it didn't warn me, everything just didn't work. A call middleware utility would solve that.

I've put together my solution (including call-middleware) in #52.

[fl0w]

I see you guys found consensus, I'm still on the defensive about either solution. Though I'll close this then, as #52's implementation is correct according to your discussion.

[fl0w]

For what it's worth; despite the aggressiveness (breaking) of @jonathanong's proposal, I feel this would be the best solution long-term.

[PlasmaPower]

@fl0w I wouldn't mind @jonathanong's proposal if we implement it using the .onUse hook that I mentioned earlier. That or #52 I'd be equally fine with. Would you be fine with the former?

[fl0w]

@PlasmaPower I think koajs/koa#707 is a step in the right direction. I'm not sure I understand the implementation yet. It definitely doesn't feel as misplaced (to me) as previous solutions.

Would it be possible and make sense to extract koajs/koa#707 (or equivalent) into its own project/package? .onUse would then effectively be part of the Koa mw signature (edit: or spec rather), which is easy to reason about.

[PlasmaPower]

@fl0w we could package prepareMiddleware into it's own package, but it wouldn't matter that much since everybody will end up using prepareMiddleware bound to the useContext. Besides, it's 15 lines of code. Is there really any reason to put it in it's own package?

Also, about how .onUse works:

This PR implementing it for koa-compose might give you a good understanding Note: that implementation is necessary for Koa since Koa uses compose to call the middleware

mw.onUsed and app.prepareMiddleware

Middleware.onUsed is called by prepareMiddleware. The prepareMiddleware chain is started in callback in callback. koa-router, koa-route, koa-compose, and koa-mount all need to implement onUsed. When onUsed is called, the application receives a "use context" defined here. This includes useCtx.prepareMiddleware. Here is an example use of useCtx.prepareMiddleware. As you can see, you simply pass useCtx.prepareMiddleware the middleware you want to prepare, and the name of your library. The name is used for the useChain, detailed below:

The useChain

The useChain is another component of a useContext that I put in so that profiling can have a tree. The useChain is initialized blank, but prepareMiddleware(compose, ..., 'koa') means that useChain will quickly be added 'koa'. After that, remember that we are preparing compose. Compose adds 'compose' to the useChain. That means that if you do app.use(middleware), middleware.onUsed will be called with a useChain of ['koa', 'compose']. Note that each useChain is a unique array of course (concat creates a new array). TODO: investigate if we should use the middleware function itself instead of a name, that would work better for profiler trees I think.

Edit: I've since changed the useChain to be made up of middleware functions instead of names.