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

Document event arguments (http2, maybe more) #877

Closed
grantila opened this issue Oct 11, 2017 · 31 comments
Closed

Document event arguments (http2, maybe more) #877

grantila opened this issue Oct 11, 2017 · 31 comments

Comments

@grantila
Copy link

  • Scope (install, code, runtime, meta, other?):
    Documentation

  • Module (and version) (if relevant):
    http2

When reading through parts of the http2 documentation, I'm missing information about what arguments are passed in many events. It's like documenting that there's a function called foo but not what argument it takes.

If an event is emitted without any arguments, writing "Emitted without any arguments" in the documentation would be helpful. I tend to console.log(arguments) just to see what's there, and this forces me to try to cause events to actually be emitted... I'm probably not the only one.

session:close 😟 I'm guessing none?
session:connect 😟 None? Also, what will I typically listen to?
session:error 😧 An error argument perhaps? No?
session:frameError 😉 Well documented! But, the argument types have a different layout compared with:
session:goaway 👍
session:localSettings 😕 What properties can I expect?
session:remoteSettings 😕 What properties can I expect?
session:stream 😔 Flags? This is my flag: 🇸🇪
session:socketError 😰 What am I supposed to do?
session:timeout 😒 Doesn't look like any arguments by the example snippet. And should I expect Node to close the socket?

The same goes for the streams...

stream:aborted 😶 "Writable end". Is that my end, or the remote peers? Is abortion an error, i.e. should I expect an argument?
stream:error 🤒 No argument in an error event. Or? Probably there is one...
stream:frameError 🤔 Interesting...
stream:streamClosed 🤗 Actually useful description, since it describes how this event relates to the flow of other events.
stream:timeout 🙁 What, am I supposed to do? I read the doc for setTimeout and it seems I should manually perform things in this event handler...
stream:trailers 😂 More flags! 🏳️‍🌈

I realize http2 is in early stage. But for early adopters and testers, this kind of information is very useful if it is provided. I've seen this lack of information before in other modules, so probably the documentation needs an overhaul of the events specifically, with exactly what is being sent.

@gireeshpunathil
Copy link
Member

@grantila - is this still outstanding? are you in a position to contribute to the improvement on the documentation?

@gireeshpunathil
Copy link
Member

Ping @nodejs/http2 to figure out next action here

@mcollina
Copy link
Member

@gireeshpunathil it would be fantastic if someone went through that list and opened issues (maybe with good-first-contribution label) for all of the events that are missing.

@ryzokuken
Copy link

ryzokuken commented Apr 12, 2018

@mcollina I could try opening a few by this weekend.

@mcollina
Copy link
Member

awesome! Remember to tag @nodejs/http2 in all of them.

@ryzokuken
Copy link

ryzokuken commented Apr 13, 2018

Checklist for reference

Http2Session

  • session:close
  • session:connect
  • session:error
  • session:frameError
  • session:goaway
  • session:localSettings
  • session:remoteSettings
  • session:stream
  • session:socketError
  • session:timeout

Http2Stream

  • stream:aborted
  • stream:error
  • stream:frameError
  • stream:streamClosed
  • stream:timeout
  • stream:trailers

Final

  • Final cleanup PR that cleans up the docs and makes all the argument listings consistent. Resolves this issue.

ryzokuken added a commit to ryzokuken/node that referenced this issue Apr 21, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)
ryzokuken added a commit to nodejs/node that referenced this issue Apr 21, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20193
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
ryzokuken added a commit to ryzokuken/node that referenced this issue Apr 21, 2018
Add parameters for the callback for the Http2Session:error event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)
ryzokuken added a commit to nodejs/node that referenced this issue Apr 23, 2018
Add parameters for the callback for the Http2Session:error event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20206
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
jasnell pushed a commit to nodejs/node that referenced this issue Apr 23, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20193
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
jasnell pushed a commit to nodejs/node that referenced this issue Apr 23, 2018
Add parameters for the callback for the Http2Session:error event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20206
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
ryzokuken added a commit to ryzokuken/node that referenced this issue Apr 24, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)
ryzokuken added a commit to nodejs/node that referenced this issue Apr 25, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20236
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
ryzokuken added a commit to ryzokuken/node that referenced this issue Apr 26, 2018
Improve parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)
ryzokuken added a commit to nodejs/node that referenced this issue Apr 27, 2018
Improve parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)
ryzokuken added a commit to ryzokuken/node that referenced this issue Apr 27, 2018
Add parameters for the callback for the Http2Session:localSettings event
and Http2Session:remoteSettings event inline with the pattern in the rest
of the documentation.

Refs: nodejs/help#877 (comment)
ryzokuken added a commit to nodejs/node that referenced this issue Apr 30, 2018
Add parameters for the callback for the Http2Session:localSettings event
and Http2Session:remoteSettings event inline with the pattern in the
rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20371
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue May 4, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20236
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue May 4, 2018
Improve parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)
@dev-script
Copy link

I want to work on all pending issues. Please allow me. Thanks

gireeshpunathil pushed a commit to nodejs/node that referenced this issue Oct 31, 2019
Add line in doc/http2.md for 'timeout' event which tell readers
that 'timeout' event doesn't except any arguments.

Refs: nodejs/help#877
PR-URL: #30161
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@dev-script
Copy link

dev-script commented Oct 31, 2019

Unable to find stream:streamClosed event. Above mentioned link open Node.js v8.16.2 Documentation page.

gireeshpunathil pushed a commit to nodejs/node that referenced this issue Nov 3, 2019
Line added in the description of http2 aborted event
that it's listener does not expect any arguments.

Refs: nodejs/help#877
PR-URL: #30179
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 5, 2019
Add line in doc/http2.md for 'timeout' event which tell readers
that 'timeout' event doesn't except any arguments.

Refs: nodejs/help#877
PR-URL: #30161
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 5, 2019
Line added in the description of http2 aborted event
that it's listener does not expect any arguments.

Refs: nodejs/help#877
PR-URL: #30179
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 8, 2019
Add line in doc/http2.md for 'timeout' event which tell readers
that 'timeout' event doesn't except any arguments.

Refs: nodejs/help#877
PR-URL: #30161
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 8, 2019
Line added in the description of http2 aborted event
that it's listener does not expect any arguments.

Refs: nodejs/help#877
PR-URL: #30179
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 10, 2019
Add line in doc/http2.md for 'timeout' event which tell readers
that 'timeout' event doesn't except any arguments.

Refs: nodejs/help#877
PR-URL: #30161
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 10, 2019
Line added in the description of http2 aborted event
that it's listener does not expect any arguments.

Refs: nodejs/help#877
PR-URL: #30179
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 10, 2019
Add line in doc/http2.md for 'timeout' event which tell readers
that 'timeout' event doesn't except any arguments.

Refs: nodejs/help#877
PR-URL: #30161
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 10, 2019
Line added in the description of http2 aborted event
that it's listener does not expect any arguments.

Refs: nodejs/help#877
PR-URL: #30179
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 11, 2019
Add line in doc/http2.md for 'timeout' event which tell readers
that 'timeout' event doesn't except any arguments.

Refs: nodejs/help#877
PR-URL: #30161
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 11, 2019
Line added in the description of http2 aborted event
that it's listener does not expect any arguments.

Refs: nodejs/help#877
PR-URL: #30179
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Dec 10, 2019
Document arguments for the 'frameError', 'timeout' and 'trailers'
event.

PR-URL: #30373
Fixes: nodejs/help#877
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@gireeshpunathil
Copy link
Member

Looks like this got closed because of the Fixes tag in nodejs/node#30373

@dev-313 - can you pls confirm all the items that are listed in the checklist (#877 (comment)) are covered? If not, are you going to plan to work on those?

Keeping it open until then.

@dev-script
Copy link

Looks like this got closed because of the Fixes tag in nodejs/node#30373

@dev-313 - can you pls confirm all the items that are listed in the checklist (#877 (comment)) are covered? If not, are you going to plan to work on those?

Keeping it open until then.

ok @gireeshpunathil

@dev-script
Copy link

dev-script commented Dec 11, 2019

Checklist for reference

Http2Session

* [x]  session:close

* [x]  session:connect

* [x]  session:error

* [x]  session:frameError

* [x]  session:goaway

* [x]  session:localSettings

* [x]  session:remoteSettings

* [x]  session:stream

* [ ]  ~session:socketError~

* [ ]  session:timeout

Http2Stream

* [ ]  stream:aborted

* [ ]  stream:error

* [ ]  stream:frameError

* [ ]  stream:streamClosed

* [ ]  stream:timeout

* [ ]  stream:trailers

Final

* [ ]  Final cleanup PR that cleans up the docs and makes all the argument listings consistent. Resolves this issue.

Checklist for remaining tasks

Http2Session

  • session:timeout

Http2Stream

  • stream:aborted

  • stream:error (Https2Stream 'error' event same as Https2Session so i think no need to change.)

  • stream:frameError

  • stream:streamClosed (it might have been removed.)

  • stream:timeout

  • stream:trailers

@dev-script
Copy link

dev-script commented Dec 13, 2019

@gireeshpunathil All remaining tasks are completed but can you please help me to understand last task "Final PR".

@gireeshpunathil
Copy link
Member

thanks @dev-313 (and others) for doing this!

Final PR as referred in #877 (comment) would mean to go through the http2.md doc and making sure the doc text is consistent (wordings, construct, style etc.) and if not, make so. I don't think we need to hold up this issue for that; so will close this. If you find anything that needs change, please go ahead a raise a PR; or else, that is it!

thanks again!

@dev-script
Copy link

Thanks @gireeshpunathil. If i find anything that need change, I'll raise a PR.

targos pushed a commit to nodejs/node that referenced this issue Jan 14, 2020
Document arguments for the 'frameError', 'timeout' and 'trailers'
event.

PR-URL: #30373
Fixes: nodejs/help#877
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this issue Feb 6, 2020
Document arguments for the 'frameError', 'timeout' and 'trailers'
event.

PR-URL: #30373
Fixes: nodejs/help#877
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants