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

[breaking change] don't bind onerror to context #800

Merged
merged 1 commit into from Aug 29, 2016
Merged

Conversation

dead-horse
Copy link
Member

  1. People should understand javascript's this, and it's easy to handle with arrow functions just like stream.on('error', err => this.onerror(err));.
  2. We already handled all the errors automatically in koa, 99.9% people won't use this.onerror in app level. We have written 100+ apps and middlewares in production, no one used this.onerror.
  3. It's has higher performance(~10% increase).
  • before
  1 middleware
  9134.88

  5 middleware
  9209.45

  10 middleware
  8261.05

  15 middleware
  8873.18

  20 middleware
  8957.63

  30 middleware
  9036.78

  50 middleware
  8543.95

  100 middleware
  8088.44
  • after
  1 middleware
  9939.28

  5 middleware
  9806.14

  10 middleware
  9636.60

  15 middleware
  9603.57

  20 middleware
  9664.59

  30 middleware
  9695.19

  50 middleware
  9220.74

  100 middleware
  8498.86

@dead-horse dead-horse added this to the v2.0.0 milestone Aug 27, 2016
@codecov-io
Copy link

codecov-io commented Aug 27, 2016

Current coverage is 100% (diff: 100%)

Merging #800 into v2.x will not change coverage

@@           v2.x   #800   diff @@
==================================
  Files         4      4          
  Lines       415    415          
  Methods      80     80          
  Messages      0      0          
  Branches    100    100          
==================================
  Hits        415    415          
  Misses        0      0          
  Partials      0      0          

Powered by Codecov. Last update 614b4e1...bfab79e

@dead-horse
Copy link
Member Author

@koajs any thoughts?

@tj
Copy link
Member

tj commented Aug 27, 2016

👍 kinda weird that it's pre-bound, onerror is private anyway so it should be fine

@fengmk2
Copy link
Member

fengmk2 commented Aug 28, 2016

+1 for the considerable performance increase.

onFinished(res, ctx.onerror);
fn(ctx).then(() => respond(ctx)).catch(ctx.onerror);
onFinished(res, err => ctx.onerror(err));
fn(ctx).then(() => respond(ctx)).catch(err => ctx.onerror(err));
Copy link
Member

Choose a reason for hiding this comment

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

Create onerror handler once should be better.

const onerror = err => ctx.onerror(err);
onFinished(res, onerror);
fn(ctx).then(() => respond(ctx)).catch(onerror);

@juliangruber
Copy link
Contributor

👍

@dead-horse dead-horse merged commit 4338cb6 into v2.x Aug 29, 2016
@dead-horse dead-horse deleted the onerror-not-bind branch August 29, 2016 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants