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

function *(next) middleware signature #1

Merged
merged 10 commits into from
Nov 8, 2013
Merged

function *(next) middleware signature #1

merged 10 commits into from
Nov 8, 2013

Conversation

jonathanong
Copy link
Member

wooo! if we merge this, middleware signature would be:

app.use(function *(next) {
  // do stuff
  yield next();
  // do stuff
})

if you want to delegate middleware, you would have to do:

app.use(function *(next) {
  if (true) yield fn.call(this, next);
})

we could add a context helper or do next(fn).

it kind of has an error check when there are double next()s. it will also work only if you do yield next();, not return next() or yield next, which is more strict than before, a good thing.

this is much more elegant than the other versions we tried. plus, i didn't like special casing 'next'. it's also easier to use:

co.call(ctx, compose(app.middleware))(ctx.onerror);

(unless you want to memoize the composition, which i would prefer).

@tj
Copy link
Member

tj commented Nov 6, 2013

the problem with this is that it still only does the linear walk, more like Connect, so we'd have to do the more complex dispatch that yields control back to each one in reverse like we had before

@jonathanong
Copy link
Member Author

can you be specific? i can't think of a case that koa currently covers but this doesn't. i should add tests for errors, though.

@tj
Copy link
Member

tj commented Nov 6, 2013

ill take another look in the morning maybe i read it wrong

@eivindfjeldstad
Copy link
Member

nice!

@tj
Copy link
Member

tj commented Nov 6, 2013

@jonathanong yeah the problem is like in our original attempt to do this sort of syntax, where you have to manually perform the "stack" behaviour that is naturally occurring in what we have implemented right now. This PR loops forward but doesn't loop in reverse to resume generators the second time for the "unwinding" portion of the stack. We could definitely do it, but there's certainly a decrease in elegance from a composition/internal point a view (I'm not biased to either really)

@jonathanong
Copy link
Member Author

Is there a test case for what you're talking about? Not sure what unwinding the stack is because I don't have a CS background so my jargon is limited. Would help if you had tests for compose as well :P

@tj
Copy link
Member

tj commented Nov 6, 2013

haha yeah tests would help. ill dig into this in a little bit

@tj
Copy link
Member

tj commented Nov 6, 2013

finally working on a real-world koa app :D haha

@tj
Copy link
Member

tj commented Nov 6, 2013

koajs/route is one example of why I don't think we reallllly need to go this route, once a base set of decent middleware are established people won't really be writing them often, so it may be overkill to try and optimize for that, but we'll see

@jonathanong
Copy link
Member Author

you making the component registry a koa app? or slate? or both!?

@jonathanong
Copy link
Member Author

yeah, especially since i would be using context methods way more than middleware unlike connect/express.

@tj
Copy link
Member

tj commented Nov 6, 2013

neither of those two actually but maybe the registry later. yeah totally, yield makes that stuff a lot nicer

@jonathanong
Copy link
Member Author

just realized. if we do:

function *next(){
  yield middleware[i++].call(this, next);
}

then we can do yield next without the () like before. not sure if we should though.

@eivindfjeldstad
Copy link
Member

maybe I'm misunderstanding, but isn't the unwinding covered here?

arr.should.eql([1, 2, 3, 4, 5, 6]);

@jonathanong
Copy link
Member Author

yes, i assume so, which is why i'm confused :P i added an error handling test as well. not sure what other cases there are.

@tj
Copy link
Member

tj commented Nov 7, 2013

nvm yeah you guys are right I read the implementation wrong haha

@tj
Copy link
Member

tj commented Nov 7, 2013

going to see what kind of impact this has on the benchmarks but otherwise looks good!

@jonathanong
Copy link
Member Author

try this too:

function *next(){
  yield middleware[i++].call(ctx, next);
}

might be a little slower, but it should be backwards compatible (so we can delay changing everything for now).

@tj
Copy link
Member

tj commented Nov 7, 2013

with this branch:

 1 middleware
  6067.10

  5 middleware
  5930.04

  10 middleware
  5557.90

  15 middleware
  5511.37

  20 middleware
  5146.46

  30 middleware
  4879.69

  50 middleware
  4375.36

  100 middleware
  3409.37

koa's current master:

  1 middleware
  6029.76

  5 middleware
  5827.56

  10 middleware
  5302.47

  15 middleware
  4776.80

  20 middleware
  4504.73

  30 middleware
  4363.10

  50 middleware
  3834.06

  100 middleware
  3341.39

more or less the same :D

@tj
Copy link
Member

tj commented Nov 8, 2013

pretty funny how it's backwards compatible due to how co works hahha

@tj
Copy link
Member

tj commented Nov 8, 2013

we might want to warn when people pass a non-generator function that is a bit of a silent WTF, I'm just refactoring koa's tests then we just have to update docs and it should be good to go

tj added a commit that referenced this pull request Nov 8, 2013
function *(next) middleware signature
@tj tj merged commit 7c54e09 into master Nov 8, 2013
@tj tj deleted the gen-next branch November 8, 2013 00:09
@jonathanong
Copy link
Member Author

Hmmmm I wonder why the dispatcher was so much faster

@tj
Copy link
Member

tj commented Nov 9, 2013

a lot less function calls, especially since this goes through co now and does all the evil-but-necessary try/catches. pretty happy with it though, im sure we can make some pretty simple improvements to get some 'free' wins

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.

3 participants