Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrap mw w/ all provided wrappers from mw._wrappers #55

Closed
wants to merge 4 commits into from
Closed

Wrap mw w/ all provided wrappers from mw._wrappers #55

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a961e6a
wrap mw w/ all provided wrappers from mw._wrappers
fl0w Apr 25, 2016
e29fe2c
fixes linting errors
fl0w Apr 25, 2016
39a2ad9
typo - because I dont test these prototype PRs
fl0w Apr 25, 2016
de8038c
extract, and solidify wrapping
fl0w May 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ function compose (middleware) {
function dispatch (i) {
if (i <= index) return Promise.reject(new Error('next() called multiple times'))
index = i
const fn = middleware[i] || next
const fn = wrap(middleware[i]) || next
Copy link
Contributor

@PlasmaPower PlasmaPower May 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The performant solution would be this:

Object.defineProperty('_wrappers', {
  set: wrappers => middleware = wrappers.reduce((mw, wrapper) => wrapper(mw), middeware),
  get: []
})

That would be a breaking change though, even if we properly implement get as setting wrappers multiple times would wrap the middleware multiple times. That could also be changed with an if statement of course, but I think we should choose a style to implement it with and keep it consistent.

On the other hand, this was supposed to be the simpler solution and if we implement it with defineProperty it might as well be a function.

if (!fn) return Promise.resolve()
try {
return Promise.resolve(fn(context, function next () {
Expand All @@ -49,3 +49,21 @@ function compose (middleware) {
}
}
}

/**
* composes a wrapped middleware using middleware._wrappers
*
* @param {Function} middleware
* @return {Function} composed
* @api public
*/

function wrap (middleware) {
if (!middleware) return false
if (!middleware._wrappers) return middleware
if (!Array.isArray(middleware._wrappers)) throw new TypeError('._wrappers must be an array')
if (!middleware._wrappers.length) return middleware
const wrappers = middleware._wrappers
delete middleware._wrappers
return compose([...wrappers, middleware])
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't how we want the wrappers to work, correct? I think you should do return wrappers.reduce((mw, wrapper) => wrapper(mw), middeware). Also, you should put this in either a separate library or Koa.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is a lot of logic. It has to be run every single time a middleware is called correct? Yet another reason to wrap middleware before runtime IMO.

Copy link
Contributor Author

@fl0w fl0w May 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It is a lot of logic, yes, but it's only an init-overhead. It's only run on compose, so in essence once Koa composes in koa/Application#callback. I'm not to worried about this overhead initially.

Here's the thing, there's a difference between a function that returns a wrapped mw, and a stack of mw that unwraps.

As a result, this PR will not solve the usage of koa-convert.

That's why my previous PR (which you correctly critiqued) had issues - I kept messing up between the types. I'll do a full spec/explanation according to Jonathans questions in the other issue (somewhere).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not only an init-overhead - look at where the function is called - every time any middleware is executed, this function is called.

Also, I'm pretty sure you don't want to use compose internally here, instead use reduce. Look at my previous comment. That is, unless you want to change the wrapper style to something not really like a wrapper, more like a mw that runs before every mw.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, using ... for array concatenation isn't supported on Node v4 (causing the tests to fail).

}