-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Comments
It's also in the wrong place in the documentation, alphabetically speaking. |
@arei : Thanks for your suggestions first!
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 :) |
/cc:@vsemozhetbyt, @mcollina, @Trott |
I think it would be better to explicitly state the signature of the callback function, e.g. |
#### 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`. |
Yes, I've prepared to describe this in details in two parts:
|
Waiting for others' suggestions from this submit :) |
@nodejs/http2 @nodejs/documentation |
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>
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>
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>
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>
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>
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>
This was resolved |
This documentation change is incorrect. The prototype given for the callback states in the itemized list that the second argument is: But the code does, on successful creation of the stream: node/lib/internal/http2/compat.js Line 798 in 21f2e88
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. |
Refs: nodejs#22322 (comment) PR-URL: nodejs#36631 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: #22322 (comment) PR-URL: #36631 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: #22322 (comment) PR-URL: #36631 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
http2: documentation for
createPushResponse()
inHttp2ServerResponse
should spell out what its callback returns more clearly. My interpretation was that it would return similar toServerHttp2Stream.pushResponse()
but it does not. In specific, headers are not passed to the callback ofcreatePushResponse()
.node/lib/internal/http2/compat.js
Line 670 in fc84666
The text was updated successfully, but these errors were encountered: