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.

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

wants to merge 4 commits into from

Conversation

fl0w
Copy link
Contributor

@fl0w fl0w commented Apr 25, 2016

Try 3456.

This adds a dependency by checking for wrappers at middleware._wrappers.
This solves a few things. 3rd party authors need not to necessarily do any change.

Consumers can "solve" this by appending ._wrappers manually if needed:

const myMw = async (ctx, next) => { /* ... */ }
myMw._wrappers([wrapper1, wrapper2])

There's no signature change for function compose(), and no weird circular dependency from either Koa or its context.

Optional
If we want, Koa can now extend koa/Application.use with an optional options argument, to be consumed as following:

app.use(mw, {
  name: 'MyMiddleware', // or manually `mw._name = 'MyMiddleware`
  wrappers: true, // wraps using `this.wrappers`
  // or negate
  wrappers: false, // default true
  // or for a specific use-case, such as @PlasmaPower's pet peeve to extract auto convert
  // into a wrapper
  wrappers: [wrapper1, wrapper2, convert]
})

Or manually, because koa-router don't implement this yet:

let myMw = module.exports =  async (ctx, next) => {
  // ...
}

myMw._name = 'MyMiddleware'
myMw._wrappers = [wrapper1, wrapper2, convert]

Accompanied PR to Koa in a second.
Edit: koajs/koa#718 for Koa implementation (without the optional stuff)

@fl0w
Copy link
Contributor Author

fl0w commented Apr 25, 2016

Btw, can we adopt standard in koa so my mind doesn't explode each PR :) ?

@codecov-io
Copy link

codecov-io commented Apr 25, 2016

Current coverage is 80.77%

Merging #55 into next will increase coverage by -19.23%

@@            next        #55   diff @@
=======================================
  Files          1          1          
  Lines         18         26     +8   
  Methods        0          5     +5   
  Branches       5          9     +4   
=======================================
+ Hits          18         21     +3   
- Misses         0          5     +5   
  Partials       0          0          

Powered by Codecov. Last updated by 8c7e940...c3c23bc

@fl0w fl0w changed the title wrap mw w/ all provided wrappers from mw._wrappers Wrap mw w/ all provided wrappers from mw._wrappers Apr 25, 2016
@haoxins
Copy link
Member

haoxins commented Apr 25, 2016

Btw, can we adopt standard in koa so my mind doesn't explode each PR :)

Most people would think that is noisy IMO. 😢

@PlasmaPower
Copy link
Contributor

I don't like that you can't achieve this at compile time without defining a getter, it seems sort of hacky.

Side note: is the codecov image way too large for anyone else?

if (!fn) return Promise.resolve()
if (fn._wrappers && Array.isArray(fn._wrappers) && fn._wrappers.length) {
fn = fn._wrappers.foreach((wrapper) => { fn = wrapper(fn) })
Copy link
Contributor

@PlasmaPower PlasmaPower Apr 25, 2016

Choose a reason for hiding this comment

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

I'm pretty sure this won't work, have you tested it with two composes on top of koa itself? Have you tested it at all? A lot of things seem off - I think you're confusing fn (the middleware being processed) with the middleware that compose returns.

@PlasmaPower
Copy link
Contributor

PlasmaPower commented Apr 25, 2016

can we adopt standard in koa

I far prefer koa's styling to standard, besides semicolons the fact that I have to do wrappers.forEach((wrapper) => { mw = wrapper(mw) }) instead of wrappers.forEach(wrapper => mw = wrapper(mw)) is just silly IMO.

Either way, I would like to standardize on something, but this is the secondary and smaller codebase so IMO it should bend to koajs/koa. Just my opinion though.

@fl0w
Copy link
Contributor Author

fl0w commented Apr 25, 2016

Ugh, that's for forcing a push be4 flight :/ I think I rebased wrong branch. I'll update this in the morning with correct stuff. Sorry about that.

@fl0w
Copy link
Contributor Author

fl0w commented Apr 25, 2016

And yea, that codecov image ...

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

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).

@fl0w fl0w closed this Jun 7, 2017
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.

4 participants