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

createPushResponse() callback needs better documentation #22322

Closed
arei opened this issue Aug 14, 2018 · 10 comments
Closed

createPushResponse() callback needs better documentation #22322

arei opened this issue Aug 14, 2018 · 10 comments
Labels
doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem.

Comments

@arei
Copy link

arei commented Aug 14, 2018

  • Version: 10.8.0
  • Platform: docs
  • Subsystem: http2

http2: documentation for createPushResponse() in Http2ServerResponse should spell out what its callback returns more clearly. My interpretation was that it would return similar to ServerHttp2Stream.pushResponse() but it does not. In specific, headers are not passed to the callback of createPushResponse().

callback(null, new Http2ServerResponse(stream));

@ChALkeR ChALkeR added http2 Issues or PRs related to the http2 subsystem. doc Issues and PRs related to the documentations. labels Aug 15, 2018
@arei
Copy link
Author

arei commented Aug 16, 2018

It's also in the wrong place in the documentation, alphabetically speaking.

@ghost
Copy link

ghost commented Aug 17, 2018

@arei

Thanks for your suggestions first!

It's also in the wrong place in the documentation, alphabetically speaking.

What about fixing like this following?

#### response.createPushResponse(headers, callback)
<!-- YAML
added: v8.4.0
-->
* `headers` {HTTP/2 Headers Object} An object describing the headers
* `callback` {Function}

Call [`http2stream.pushStream()`][] with the given headers, and wraps the
given [`Http2Stream`] on a newly created `Http2ServerResponse` as the callback
parameter when callback is successful. And for the closed stream, The callback
will be called with an error `ERR_HTTP2_INVALID_STREAM`.

Feel free to tell me if there's something wrong or should be also fixed :)

@ghost
Copy link

ghost commented Aug 17, 2018

/cc:@vsemozhetbyt, @mcollina, @Trott

@arei
Copy link
Author

arei commented Aug 17, 2018

I think it would be better to explicitly state the signature of the callback function, e.g. callback(err,stream)or even better like the callback is written in for http2stream.pushStream().

@ghost
Copy link

ghost commented Aug 17, 2018

#### response.createPushResponse(headers, callback)
<!-- YAML
added: v8.4.0
-->
* `headers` {HTTP/2 Headers Object} An object describing the headers
* `callback` {Function} Callback that is called once `[`http2stream.pushStream()`][] ` has finished, or
  when the stream is closed.
  * `err` {Error}
  * `stream` {ServerHttp2Stream} The newly-created `ServerHttp2Stream` object.

Call [`http2stream.pushStream()`][] with the given headers, and wraps the
given [`Http2Stream`] on a newly created `Http2ServerResponse` as the callback
parameter when callback is successful. And for the closed stream, The callback
will be called with an error `ERR_HTTP2_INVALID_STREAM`.

@ghost
Copy link

ghost commented Aug 17, 2018

@arei

Yes, I've prepared to describe this in details in two parts:

  1. Simple introductions of the callback :including each of the callback parameters.
  2. Details info: What will happen for a successful callback and what's with that when failed.

@ghost
Copy link

ghost commented Aug 17, 2018

Waiting for others' suggestions from this submit :)

@Trott
Copy link
Member

Trott commented Aug 17, 2018

@nodejs/http2 @nodejs/documentation

mcollina pushed a commit that referenced this issue Aug 23, 2018
We don't know what will return when successful or failure for
the callback of the function. So this commit makes it more detailled.

PR-URL: #22366
Refs: #22322
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this issue Aug 24, 2018
We don't know what will return when successful or failure for
the callback of the function. So this commit makes it more detailled.

PR-URL: #22366
Refs: #22322
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this issue Sep 3, 2018
We don't know what will return when successful or failure for
the callback of the function. So this commit makes it more detailled.

PR-URL: #22366
Refs: #22322
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Oct 3, 2018
We don't know what will return when successful or failure for
the callback of the function. So this commit makes it more detailled.

PR-URL: nodejs#22366
Refs: nodejs#22322
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Oct 16, 2018
We don't know what will return when successful or failure for
the callback of the function. So this commit makes it more detailled.

PR-URL: nodejs#22366
Refs: nodejs#22322
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this issue Oct 17, 2018
We don't know what will return when successful or failure for
the callback of the function. So this commit makes it more detailled.

Backport-PR-URL: #22850
PR-URL: #22366
Refs: #22322
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 19, 2018

This was resolved

@jasnell jasnell closed this as completed Oct 19, 2018
@ericrannaud
Copy link

ericrannaud commented Dec 22, 2020

This documentation change is incorrect. The prototype given for the callback states in the itemized list that the second argument is:
stream {ServerHttp2Stream} The newly-created ServerHttp2Stream object

But the code does, on successful creation of the stream:

callback(null, new Http2ServerResponse(stream));

So the second argument of the callback is a Http2ServerResponse, not the underlying ServerHttp2Stream. The doc correctly states this in the rest of the text.

Trott added a commit to Trott/io.js that referenced this issue Dec 27, 2020
Refs: nodejs#22322 (comment)

PR-URL: nodejs#36631
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 12, 2021
Refs: #22322 (comment)

PR-URL: #36631
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue May 1, 2021
Refs: #22322 (comment)

PR-URL: #36631
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@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. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants