-
Notifications
You must be signed in to change notification settings - Fork 147
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
Conversation
Btw, can we adopt |
Current coverage is 80.77%@@ 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
|
Most people would think that is noisy IMO. 😢 |
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) }) |
There was a problem hiding this comment.
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.
I far prefer koa's styling to standard, besides semicolons the fact that I have to do 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. |
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. |
And yea, that codecov image ... |
if (!middleware._wrappers.length) return middleware | ||
const wrappers = middleware._wrappers | ||
delete middleware._wrappers | ||
return compose([...wrappers, middleware]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
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:There's no signature change for
function compose()
, and no weird circular dependency from either Koa or itscontext
.Optional
If we want, Koa can now extend
koa/Application.use
with an optional options argument, to be consumed as following:Or manually, because
koa-router
don't implement this yet:Accompanied PR to Koa in a second.
Edit: koajs/koa#718 for Koa implementation (without the optional stuff)