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

Clarify error events on HTTP module documentation #4275

Closed
wants to merge 2 commits into from

Conversation

lmarkus
Copy link

@lmarkus lmarkus commented Dec 14, 2015

Minor clarification on how errors surface on the HTTP module.
It's not clear that if a listener is not registered, the error will be thrown.
Bumped against it this weekend.

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Dec 14, 2015
@jasnell jasnell added http Issues or PRs related to the http subsystem. lts-watch-v4.x labels Dec 14, 2015
@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

LGTM

@mscdex
Copy link
Contributor

mscdex commented Dec 14, 2015

It might be worth prefixing the addition with something like "As with all 'error' events, " to make it clear that this is not specific to http, but is true for any EventEmitter.

@lmarkus
Copy link
Author

lmarkus commented Dec 14, 2015

Makes sense. I'll update it.

@mscdex
Copy link
Contributor

mscdex commented Dec 14, 2015

LGTM

@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

Slight line wrapping nit but I'll fix that when landing...

@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

Landed in 584f636

@lmarkus ... note that I fixed up the commit log message to adhere to the style standards.

@jasnell jasnell closed this Dec 14, 2015
jasnell pushed a commit that referenced this pull request Dec 14, 2015
Make it clear that error with throw if error listener is
not registered.

PR-URL: #4275
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

Ugh.. flubbed the commit. Landed in 9538fd0

@lmarkus
Copy link
Author

lmarkus commented Dec 14, 2015

Thanks for the fast turnaround.

cjihrig pushed a commit that referenced this pull request Dec 15, 2015
Make it clear that error with throw if error listener is
not registered.

PR-URL: #4275
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Dec 30, 2015
Make it clear that error with throw if error listener is
not registered.

PR-URL: #4275
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg
Copy link
Member

rvagg commented Jan 4, 2016

Hey @lmarkus! I believe this is your first commit to core, glad to see you got around to it! Thanks for contributing, keep up the great work @ PayPal and I hope to see your name show up more around here!

@lmarkus
Copy link
Author

lmarkus commented Jan 4, 2016

Thanks @rvagg! Pretty simple as contributions go, but hopefully there will be more to come 😄

MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
Make it clear that error with throw if error listener is
not registered.

PR-URL: #4275
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Make it clear that error with throw if error listener is
not registered.

PR-URL: nodejs#4275
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants