Skip to content

Conversation

fl0w
Copy link
Contributor

@fl0w fl0w commented Jun 18, 2017

This is my take on the "wrapper hook/talk". It's minimally invasive.

The idea is to follow this up with:

  • koajs/koa:Application.use() accepts arrays with middleware, e.g. app.use([mw1, mw2]).
  • Optional: [koajs/koa:Application.wrap()] to globally wrap all this.middleware with whatever's inside this.wrappers with a simple map returning [[wrapper1, mw], [wrapper1, mw2]].

As far as I can foresee, this should be unrestrictive enough for developers to be creative as one see fit, simple e.g:

// stupid example
const wrappers = []
const wrapMiddleware = mw => {
  if (process.env.DEBUG_MIDDLEWARE) {
    return [...wrappers, mw]
  }
  return mw
}

...

const compress = require('koa-compress')
app.use(wrapMiddleware(compress()))

Related issues:
koajs/compose: #6
koajs/koa: koajs/koa#219

Related PRs:
koajs/compose: #52
koajs/compose: #53
koajs/koa: koajs/koa#707
koajs/koa: koajs/koa#694 introduces context.wrappers, but I personally would enjoy context not being part of the solution.

@codecov
Copy link

codecov bot commented Jun 18, 2017

Codecov Report

Merging #81 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #81   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          18     21    +3     
  Branches        5      6    +1     
=====================================
+ Hits           18     21    +3
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8684618...5bb378f. Read the comment docs.

@PlasmaPower
Copy link
Contributor

PlasmaPower commented Jun 18, 2017

This isn't transient to children, correct? For instance, if Koa uses a router which uses more middleware, the wrapper will be attached to the router but not to the middleware. I'm not sure this is better than defining a helper function to wrap middleware and calling app.use on the result.

@fl0w
Copy link
Contributor Author

fl0w commented Jun 18, 2017

@PlasmaPower Yes, not transient - and I agree, not necessarily a game changing "feature", unless koa-router returned an POJO array (which it doesn't).

index = i
let fn = middleware[i]
if (isArray(fn)) fn = compose(fn)
if (i === middleware.length) fn = next
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my concern here is that we compose every time we call dispatch. this might have performance issues.

i'd prefer to flatten the array at compose(), but that might not be what you want as you won't be able to edit nested middleware during runtime (which i don't think you should be doing)

Copy link
Contributor Author

@fl0w fl0w Nov 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, the point was not to mutate middleware at runtime, but to allow consumer (Koa) to pass nested/wrapped as an array - and instead of flattening the array I simply opted to compose recursively (saw it as a quality of life feature)

But you're right, I would compose on each dispatch. Maybe minuscule penalty, but definitely unnecessary as pre-flattening would avoid that.

But, this could be easily done by consumer as well. The whole point was to allow Koa to add some clean logic to wrap each middleware, but I'm not convinced this is of concern anymore. If anything, I think this can be closed and forgotten (I already had).

@fl0w fl0w closed this Nov 27, 2017
@fl0w fl0w deleted the compose-recursive branch November 27, 2017 12:19
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

Successfully merging this pull request may close these issues.

3 participants