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.

Sign up for GitHub

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

Jump to bottom

feat: set err.headerSent before app error event emit #919

Merged
merged 2 commits into from
Feb 28, 2017
Merged

feat: set err.headerSent before app error event emit #919

merged 2 commits into from
Feb 28, 2017

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Feb 26, 2017

No description provided.

@fengmk2 fengmk2 added the bug label Feb 26, 2017
@codecov
Copy link

codecov bot commented Feb 26, 2017

Codecov Report

Merging #919 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #919   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         423    425    +2     
  Branches      100    101    +1     
=====================================
+ Hits          423    425    +2
Impacted Files Coverage Δ
lib/context.js 100% <100%> (ø) ✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d740d9b...4f5cdb8. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 26, 2017 •
edited

Codecov Report

Merging #919 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #919   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         423    425    +2     
  Branches      100    101    +1     
=====================================
+ Hits          423    425    +2
Impacted Files Coverage Δ
lib/context.js 100% <100%> (ø) ✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c28d1f...5974d45. Read the comment docs.

dead-horse
dead-horse reviewed Feb 26, 2017
View reviewed changes
test/context/onerror.js

app.on('error', err => {
err.message.should.equal('mock error');
err.headerSent.should.equal(true);
Copy link
Member

Choose a reason for hiding this comment

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

the only difference is we can get err.headerSent in app's error listener?

Copy link
Member

Choose a reason for hiding this comment

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

I think so

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

@jonathanong
Copy link
Member

More of a feature than a fix but 👍

@dead-horse dead-horse changed the title fix: set err.headerSent before app error event emit feat: set err.headerSent before app error event emit Feb 28, 2017
@dead-horse dead-horse merged commit e452b68 into master Feb 28, 2017
@dead-horse dead-horse deleted the error-headerSent branch February 28, 2017 02:53
@dead-horse
Copy link
Member

@fengmk2 make a PR to v1.x please.

@fengmk2
Copy link
Member Author

fengmk2 commented Feb 28, 2017

@dead-horse ok

fengmk2 added a commit that referenced this pull request Feb 28, 2017
@fengmk2
feat: set err.headerSent before app error event emit #919
cb6350d 
cherry-pick from #919
@fengmk2 fengmk2 mentioned this pull request Feb 28, 2017
Merged
fengmk2 added a commit that referenced this pull request Feb 28, 2017
@fengmk2
feat: set err.headerSent before app error event emit
3be61e3 
cherry-pick from #919
fengmk2 added a commit that referenced this pull request Feb 28, 2017
@fengmk2
feat: set err.headerSent before app error event emit
ce68b8f 
cherry-pick from #919
fengmk2 added a commit that referenced this pull request Feb 28, 2017
@fengmk2
feat: set err.headerSent before app error event emit
57fef55 
cherry-pick from #919
dead-horse pushed a commit that referenced this pull request Mar 2, 2017
@fengmk2 @dead-horse
feat: set err.headerSent before app error event emit (#923)
8c1c69f 
cherry-pick from #919
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathanong jonathanong jonathanong approved these changes

@dead-horse dead-horse dead-horse approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fengmk2 @jonathanong @dead-horse