doc: fix improper http.get sample code in http.markdown #4263
Conversation
hidekiy
changed the title
|
lgtm |
|
LGTM |
|
LGTM Any idea how this commit vanished if it landed in nodejs/node-v0.x-archive@6671efa edit: LGTM pending until #4263 (comment) is responded to |
|
@thealphanerd I wondered the same and assumed it was just something that got missed in convergence. Perhaps it was sitting on |
| @@ -966,6 +966,8 @@ Example: | |||
|
|
|||
| http.get("http://www.google.com/index.html", function(res) { | |||
| console.log("Got response: " + res.statusCode); | |||
| // consume response body | |||
| res.resume(); | |||
There was a problem hiding this comment.
should this be above console.log as seen in @chrisdickinson's original example?
might make sense to have the response enter flowing mode before accessing it.
There was a problem hiding this comment.
I don't think that's necessary, statusCode is a property that exists when the function is called and isn't attached to the streaming nature of res in any way (beyond this point at least). I'm more concerned with the lack lack of information on what res.resume() does but explaining it here, even "enter flowing mode", is going to detract from the point of the example and likely take up too much additional space.
There was a problem hiding this comment.
So I dug a bit further into this. The response in the callback is an instance of http.IncomingMessage... and finding that out was not exactly obvious. http.IncomingMessage implements the Readable stream interface, which is where it inherits .resume from.
Quite a bit of digging is necessary to find out what this function does. While it may not make sense to duplicate documentation, perhaps we can find a better way to discover inherited properties
Any thoughts @nodejs/documentation?
If there is an interest in this I'll move it to a separate issue to avoid derailing this PR.
There was a problem hiding this comment.
Correct. IncomingMessage would probably better be HTTPMessage.
|
Nope. Likely was just missed. Good catch.
|
|
As per last in-line comment, I'm +1 on merging this as is. It should go in to Thanks for catching this @hidekiy, I believe the original commit should have been your first ever commit to Node core but it got lost in the convergence fray, sorry about that. Welcome on board now though! |
|
Thanks for reviewing. It is very helpful to me. |
|
LGTM @thealphanerd ... is your LGTM still pending or are you ok with this going ahead and landing? If there are additional improvements that can be made they can be done separately |
|
I am cool with this landing, I'll open another issue for the outstanding improvements |
Refs: nodejs/node-v0.x-archive#25471 Refs: nodejs/node-v0.x-archive#8443 PR-URL: #4263 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in c6efd53 |
Refs: nodejs/node-v0.x-archive#25471 Refs: nodejs/node-v0.x-archive#8443 PR-URL: #4263 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs/node-v0.x-archive#25471 Refs: nodejs/node-v0.x-archive#8443 PR-URL: #4263 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs/node-v0.x-archive#25471 Refs: nodejs/node-v0.x-archive#8443 PR-URL: #4263 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs/node-v0.x-archive#25471 Refs: nodejs/node-v0.x-archive#8443 PR-URL: nodejs#4263 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
I fixed improper sample code that will potentially hang up around http agent.
This is equals to nodejs/node-v0.x-archive#25471
Related issue is nodejs/node-v0.x-archive#8443