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

doc: document the connection event for HTTP2 & TLS servers #34531

Closed
wants to merge 2 commits into from
Closed

doc: document the connection event for HTTP2 & TLS servers #34531

wants to merge 2 commits into from

Conversation

pimterry
Copy link
Member

As discussed in #34296, the documentation isn't totally clear on where 'connection' events can be manually emitted, or that doing so is an officially supported use case for those events.

This PR is a quick first step to improve that - taking the text from the same event in the HTTP module (where this is already documented) and including it in the docs for TLS servers, HTTP2 servers & HTTP2 secure servers.

I've simplified the text from the HTTP event to remove details that I'm not 100% sure are true for non-HTTP servers: setTimeout handling, and the specific class of sockets that could ever possibly be emitted here. I don't think either is required for this to be useful, but happy to add those details here too if somebody can confirm they're equally relevant & true.

Checklist

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

mcollina commented Aug 3, 2020

@mcollina
Copy link
Member

mcollina commented Aug 3, 2020

@nodejs/build can you check this?

@richardlau
Copy link
Member

@nodejs/build can you check this?

I've added a + prefix to the git ref, which should allow for non-fast forward references: https://git-scm.com/book/en/v2/Git-Internals-The-Refspec

Rerun CI-Lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4195/

@mcollina
Copy link
Member

mcollina commented Aug 3, 2020

Landed in e5dacc2

@codebytere codebytere mentioned this pull request Aug 10, 2020
codebytere pushed a commit that referenced this pull request Aug 11, 2020
PR-URL: #34531
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@pimterry pimterry deleted the connection-event-docs branch August 17, 2020 10:49
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34531
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34531
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants