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

State explicitly in docs that 'close' does not take any arguments #20018

Closed
ryzokuken opened this issue Apr 13, 2018 · 4 comments
Closed

State explicitly in docs that 'close' does not take any arguments #20018

ryzokuken opened this issue Apr 13, 2018 · 4 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. http2 Issues or PRs related to the http2 subsystem.

Comments

@ryzokuken
Copy link
Contributor

In the http2 module, the Http2Session class emits an event named close, which is documented at https://github.com/nodejs/node/blob/master/doc/api/http2.md#event-close.

As evident from the following function from lib/internal/http2/core.js, the event does not expect any arguments:

function emitClose(self, error) {
  if (error)
    self.emit('error', error);
  self.emit('close');
}

However, the current docs do a poor job of communicating this, therefore creating confusion.

Thus, the fact that the close event does not expect arguments should be explicitly mentioned in the docs.

The exact source can be found at:

node/doc/api/http2.md

Lines 126 to 131 in 6376d43

#### Event: 'close'
<!-- YAML
added: v8.4.0
-->
The `'close'` event is emitted once the `Http2Session` has been destroyed.

@ryzokuken
Copy link
Contributor Author

cc @mcollina @nodejs/http2 does this look good? Feel free to suggest any structural changes, and I'd base the remaining issues on the finalized structure.

@jasnell
Copy link
Member

jasnell commented Apr 13, 2018

This is fine. Want to open a PR?

@jasnell jasnell added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Apr 13, 2018
@ryzokuken
Copy link
Contributor Author

ryzokuken commented Apr 13, 2018

@jasnell

At nodejs/help#877 (comment), @mcollina had suggested opening a few "good first issues", and that was why I was interested in the structure of the issue (so that first time contributors find it convenient). That said, I'm willing to open a PR if that's what you're propose, but that wasn't the intent when I opened the issue.

@mcollina mcollina added the good first issue Issues that are suitable for first-time contributors. label Apr 14, 2018
@mcollina
Copy link
Member

Added the "good first issue tag"

indranil added a commit to indranil/node that referenced this issue Apr 14, 2018
Explicitly added in the docs that the close event does not expect
any arguments when invoked.

Fixes: nodejs#20018
indranil added a commit to indranil/node that referenced this issue Apr 14, 2018
Added wording to better state that it is in fact the listener
that is not expecting any arguments

Fixes: nodejs#20018
jasnell pushed a commit that referenced this issue Apr 16, 2018
Explicitly added in the docs that the close event does not expect
any arguments when invoked.

Fixes: #20018

PR-URL: #20031
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue May 1, 2018
Explicitly added in the docs that the close event does not expect
any arguments when invoked.

Fixes: nodejs#20018

PR-URL: nodejs#20031
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Aug 23, 2018
Explicitly added in the docs that the close event does not expect
any arguments when invoked.

Fixes: nodejs#20018

PR-URL: nodejs#20031
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Sep 11, 2018
Explicitly added in the docs that the close event does not expect
any arguments when invoked.

Fixes: nodejs#20018

PR-URL: nodejs#20031
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Sep 19, 2018
Explicitly added in the docs that the close event does not expect
any arguments when invoked.

Fixes: nodejs#20018

PR-URL: nodejs#20031
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Oct 16, 2018
Explicitly added in the docs that the close event does not expect
any arguments when invoked.

Fixes: nodejs#20018

PR-URL: nodejs#20031
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Oct 17, 2018
Explicitly added in the docs that the close event does not expect
any arguments when invoked.

Fixes: #20018

Backport-PR-URL: #22850
PR-URL: #20031
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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. good first issue Issues that are suitable for first-time contributors. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants