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

Why not app.use(generator) ? #34

Closed
azproduction opened this issue Aug 22, 2013 · 11 comments
Closed

Why not app.use(generator) ? #34

azproduction opened this issue Aug 22, 2013 · 11 comments

Comments

@azproduction
Copy link

Like this

app.use(function *(next) {
    var start = new Date;
    yield next;
    var ms = new Date - start;
    console.log('%s %s - %s', this.method, this.url, ms);
});

Is there a reason for that?

@tj
Copy link
Member

tj commented Aug 22, 2013

just because of how we're composing them it basically passes next to all of them first and becomes:

b(c(d(e(f(notfound)))))

I had thought about doing what you mention but it would require that we use a dispatcher

@jonathanong
Copy link
Member

+1. i find nesting another function pretty annoying, especially when we're trying to avoid callbacks.

@tj
Copy link
Member

tj commented Aug 22, 2013

dispatchers are not great though, and become a reasonable source of slow down when you start mounting a lot of apps, which is one thing I'd like to get "right" with koa. With Connect the dispatcher adds considerable overhead when you have a decent size tree of apps, and it makes composition a lot more difficult / coupled to koa, but I'll see if I can find a decent way around that. If ES6 would allow yield; as yield undefined; we could just have yield and that's it. I'll try this out again and see what impact it has, it also means we can't use co due to how it's designed

@jonathanong
Copy link
Member

Errrr yeah I just realized how complicated it would make things. Personally, I wouldn't mind performance penalties as long as they aren't crippling. We can always improve the dispatcher (or try to) but changing the middleware signature would be difficult.

@tj
Copy link
Member

tj commented Aug 22, 2013

having an intermediate is helpful for debugging though, you can print out the currently executing middleware and mutations made etc, so it might be worth the added complexity, I'll see if I can refactor koa-compose to not be disgusting haha and hopefully the accumulated perf portion is not too bad with lots of mounting, we'll find out!

I have it working with the following, but mounting would definitely be much more complex than it is now:

app.use(function *(){
  console.log('before')
  yield 'next';
  console.log('after')
})

@jonathanong
Copy link
Member

how about just allowing any "falsey" value instead of 'next'?

var next;

app.use(function *(){
  console.log('before');
  yield next;
  console.log('after');
})

meh it doesn't really matter. being able to just do yield 0 would be nice though.

@tj
Copy link
Member

tj commented Aug 23, 2013

yeah fuck it, way too awkward for mounting, we can maybe revisit this in the future, it does look nicer but implementation obscurity is probably not worth it. Closing for now

@tj tj closed this as completed Aug 23, 2013
@jonathanong
Copy link
Member

You got the code on a branch? I want to check it out.

@tj
Copy link
Member

tj commented Aug 23, 2013

@jonathanong https://github.com/koajs/koa/tree/add/dispatcher

just enough to get examples/simple.js working

@tj
Copy link
Member

tj commented Aug 23, 2013

another nice thing about the dispatcher approach though is we can remove downstream, since we can check that it's reached the furthest downstream middleware

@tj
Copy link
Member

tj commented Aug 23, 2013

maybe we'll see it changed:

screen shot 2013-08-22 at 6 33 44 pm

fl0w pushed a commit to fl0w/koa that referenced this issue Apr 18, 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

No branches or pull requests

3 participants