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

No longer warns on res.close event #22

Merged
merged 1 commit into from
Jul 29, 2019
Merged

No longer warns on res.close event #22

merged 1 commit into from
Jul 29, 2019

Conversation

msmol
Copy link

@msmol msmol commented Jun 1, 2018

See: nodejs/node#20611

res.close is always triggered and is no longer considered and error or warning condition

Addresses: #21

See: nodejs/node#20611

res.close is always triggered and is no longer considered and error
or warning condition
@mareksrom
Copy link

I think that change in #20611 is incosistent with the documentation of close event - "Indicates that the underlying connection was terminated before response.end() was called or able to flush." - this is not true anymore - event close is called always. Either the documentation should be changed or this PR should be reverted or fixed. I don't know if it is intended behaviour (then isn't it a API change - SEMVER-MAJOR?), but from my point of view only unintended side efect of the fix. Previous functionality can be achieved by testing res.finished in close event...

@msmol
Copy link
Author

msmol commented Jul 8, 2018

@mareksrom see nodejs/node#21047

@eiabea
Copy link

eiabea commented Aug 22, 2018

@tellnes any plans to merge this PR? 🙏

@michaltk
Copy link

This doesn't seem to be getting merged so forked and fixed.
https://github.com/michaltk/bunyan-middleware

@tellnes tellnes merged commit e86dc99 into tellnes:master Jul 29, 2019
@tellnes
Copy link
Owner

tellnes commented Jul 29, 2019

Sorry for the delay, merged. I'll publish to npm soon.

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.

5 participants