reference app.wrappers[] for each mw

[fl0w]

This is for koajs/compose#55

[fl0w]

I don't understand what's wrong here with this PR? Locally both make test and make lint passes, but travis throws

30:41  error  Unnecessary escape character: \"  no-useless-escape

Anyway, will add tests and docs if this PR is an acceptable approach.

[PlasmaPower]

So sort of like a onUsed hook, but a variable instead?

[fl0w]

@PlasmaPower Aye, simpler. Not necessarily better but I prefer the simple solution so I just threw this out there.

[juliangruber]

sorry but I don't get the concept of wrappers you want to introduce here. can you give a high level explanation?

[PlasmaPower]

@juliangruber Sure thing! Basically a wrapper replaces any middleware with the middleware called on the function (wrappers.forEach(wrapper => mw = wrapper(mw))). Here's how a wrapper works (this code is for Koa v2 + a wrappers patch):

app.wrappers = [co.wrap];

app.use(function * (ctx, next) {
  ctx.body = 'example';
  yield next();
});

This is also useful for profilers, for instance you can do this:

app.wrappers = [function (mw) {
  return function () {
    let start = process.hrtime();
    return Promise.resolve().then(() => mw).then(function (val) {
      let time = process.hrtime(start);
      console.log(mw._name || mw.name + ': ' + time.join('.') + ' seconds';
      return val;
    });
  };
}];

[juliangruber]

Gotcha, thanks!

What seems weird to me from an api perspective is that you use app.use() to add functions to the middleware stack, but there would set the "wrapper stack" directly via app.wrappers instead of something like app.wrap(fn).

Also, do wrappers get cascaded? For example if you have

app.wrappers = [profile];
const composed = compose([a, b]);
app.use(composed);

would you just get a profile of the composed middleware or the individual ones as well?

[juliangruber]

the biggest concern I have though is that koa sets fn._wrapper and then relies on compose to consume this "hidden" property. feels like a weird coupling of components.

[PlasmaPower]

@juliangruber there are a couple different implementation PRs around, which do you prefer? As for app.wrappers vs app.wrap(mw), I prefer the raw array because you can do app.wrappers.push(wrapper) anyway. It makes more sense to me as an array rather than a function I guess. Also arrays are a bit more versatile.

Most implementations cascade wrappers, yes. That is the expected behavior. In most implementations, that requires changes to those libraries though.

[PlasmaPower]

the biggest concern I have though is that koa sets fn._wrapper and then relies on compose to consume this "hidden" property. feels like a weird coupling of components.

Unfortunately, there doesn't seem to be a very clean way to do this. Which PR implementing wrappers do you prefer? Each has it's advantages and disadvantages IMO.

[fl0w]

@juliangruber the main problem is; how do we allow wrapping for the entire app - no matter what library user uses. It's handled by compose because had we made this a koa solution, libraries such as koa-router wouldn't be affected properly by the wrapping.

This could easily have been a koa/external solution if we made a deep tree of mw's before a compose/equivalent. Though that's not the case today.

[PlasmaPower]

@fl0w I'm still not convinced creating a tree of middleware is possible in a lot of cases without sacrificing performance.

[fl0w]

@PlasmaPower aye, maybe. My point it, I don't think we can avoid wrapping in compose, right?

[PlasmaPower]

@fl0w I'm pretty sure that's a necessary evil, yes. We can still offload the logic though, like I've done in my PR.

[juliangruber]

I'm bot sure cascading wrappers is a good idea, what happens inside a mw is its business and its business only.

[fl0w]

@juliangruber it doesn't cascade, it just flat-wraps all mw in Application.middleware?

My PR in compose is messed up - wrong rebase apparently, working together a proper one as we speak (slowly, in a meeting)

[juliangruber]

@fl0w if i have this:

app.wrappers = [(mw) => {
  return () => {
    return mw().then(console.log('hi'))
  }
}];
const composed = compose([a, b]);
app.use(composed);

will it log hi 1x or 3x?

[fl0w]

@juliangruber the goal is for compose to wrap all wrappers, once per mw. Answer: 2x.
Once for a, once for b.

The end goal is to wrap each individual mw with all provided wrappers once.

[fl0w]

The equivalent today for koa@2 could be:

const wrapper = async (ctx, next) => {
  console.log('hi')
}

const wrappedMw1 = async (ctx, next) => {
  // .. mw1 logic
  await wrapper()
}

const wrappedMw2 = async (ctx, next) => {
  // .. mw2 logic
  await wrapper()
}

app.use(wrappedMw1)
app.use(wrappedMw2)

[juliangruber]

hmmm...curious what @tj and @jonathanong think

[fl0w]

@juliangruber there's lengthy bikeshedding at koajs/compose#51 as well.

[jonathanong]

30:41 error Unnecessary escape character: " no-useless-escape

eslint update. rm -rf node_modues; npm i :)

might be a separate PR if it's failing master/next right now

[jonathanong]

there was some discussion in some PRs:

• Wrap mw w/ all provided wrappers from mw._wrappers compose#55
• Add support for a wrapper array in context compose#52

i'm not sure where i stated it, but my main concern are specifics. like @juliangruber stated, does it nest into compose? we have to spec it all out then write tests for all the cases. i haven't seen anyone either one yet

[codecov-io]

Current coverage is 100%

Merging #718 into v2.x will not change coverage

@@               v2.x       #718   diff @@
==========================================
  Files             4          4          
  Lines           414        424    +10   
  Methods          80         82     +2   
  Messages          0          0          
  Branches         99        102     +3   
==========================================
+ Hits            414        424    +10   
  Misses            0          0          
  Partials          0          0          

Powered by Codecov. Last updated by 41afac3...f132f79