doc: document res.connection and res.socket #13617

JustinBeckwith · 2017-06-11T18:22:08Z

Adds documentation for the connection and socket properties
available on the http.serverResponse object.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Trott

LGTM. (As always, there's room for some more information to be added, but this is certainly better than not having it documented at all.)

Trott · 2017-06-11T18:32:25Z

I wonder if it might be possible to indicate a use case where a user might want to access these properties. Otherwise, readers may wonder about the statements that users usually won't want to access them.

mscdex · 2017-06-11T18:45:38Z

I think we should just document one of the two. At some point it'd be nice to remove one of them to have less duplication.

Since core itself uses .socket much more than .connection, I'd say we document .socket.

JustinBeckwith · 2017-06-11T19:35:48Z

That's a great point. I was mostly just trying to address #12617, but to be honest I am having trouble thinking of an example where accessing the raw socket is required or advisable. Followed up on the issue to learn more.

dismissing my review until mscdex suggestion sorted; I wouldn't stop anyone else from approving if they like this as-is, though

JustinBeckwith · 2017-06-12T01:41:59Z

I went ahead and added the same documentation for request.socket and request.connection, since their use is probably more common than the same properties on the response.

@mscdex I agree that having both of these isn't ideal. I did some searching, and mostly found folks using req.connection as opposed to req.socket, even in the express docs:

https://www.google.com/search?q=%22req.connection%22&oq=%22req.connection&aqs=chrome.0.69i59j0j69i57j0l3.3608j0j7&sourceid=chrome&ie=UTF-8

For now, I went ahead and documented both, with connection just linking out to socket. I'm happy to file a follow up issue to make the API breaking part of the change. Thoughts?

JustinBeckwith · 2017-06-12T01:43:02Z

Also, I added an example :)

lpinca · 2017-06-12T07:57:42Z

doc/api/http.md

+added: v0.3.0
+-->
+
+* `connection` {net.Socket}


Nit: I think the name (`connection`) is not needed here as this is property not a function argument.

aqrln · 2017-06-12T10:57:28Z

doc/api/http.md

+```js
+const http = require('http');
+http.createServer((req, res) => {
+    const clientIp = req.socket.remoteAddress;


Nit: can you please use two spaces for indentation?

aqrln · 2017-06-12T10:57:39Z

doc/api/http.md

+```js
+const http = require('http');
+http.createServer((req, res) => {
+    const clientIp = res.socket.remoteAddress;


aqrln · 2017-06-12T11:09:36Z

doc/api/http.md

+
+* `socket` {net.Socket}
+
+Reference to the underlying socket. `socket` is an object of type


I don't think we need to duplicate the type in a separate sentence if it is already specified above.

JustinBeckwith · 2017-06-12T15:55:14Z

Thanks for the awesome feedback folks. All addressed.

aqrln

LGTM with a couple of tiny nits. Thanks!

aqrln · 2017-06-12T17:15:56Z

doc/api/http.md

+
+Reference to the underlying socket. Usually users will not want to access 
+this property. In particular, the socket will not emit `'readable'` events 
+because of how the protocol parser attaches to the socket. After `response.end()`, 


Can you please wrap this line and the next one it at 80 characters?

aqrln · 2017-06-12T17:16:29Z

doc/api/http.md

+
+Reference to the underlying socket. Usually users will not want to access 
+this property. In particular, the socket will not emit `'readable'` events 
+because of how the protocol parser attaches to the socket. After `response.end()`, 


jasnell

LGTM with nits addressed

silverwind · 2017-06-13T20:28:30Z

I think worthy cases to document would be req.connection.remoteAddress and req.connection.remotePort, which hold the remote adress and port, something which is useful for logging and access control.

JustinBeckwith · 2017-06-17T21:12:21Z

Addressed all the things and rebased into one commit!

addaleax · 2017-06-17T21:14:45Z

doc/api/http.md

@@ -497,6 +497,15 @@ If a request has been aborted, this value is the time when the request was
 aborted, in milliseconds since 1 January 1970 00:00:00 UTC.

 ### request.end([data[, encoding]][, callback])
+### request.connection


Are these supposed to be directly after each other?

silverwind · 2017-06-18T13:06:53Z

doc/api/http.md

+const http = require('http');
+http.createServer((req, res) => {
+  const clientIp = req.socket.remoteAddress;
+  res.end(`Your IP is ${clientIp}`);


Sorry to nitpick, but "IP address" would be more correct, as IP is just the protocol. Also, I'd add a second print for the port. Same goes for the res example below.

Adds documentation and samples for the `connection` and `socket` properties available on the `http.serverResponse` and `http.clientRequest` objects. Fixes: nodejs#12617

JustinBeckwith · 2017-06-19T06:18:37Z

One more try 😂

silverwind · 2017-06-19T16:24:26Z

Thanks! Landed in d3d66a5.

I also took the liberty to change IP to IP address 😉

Adds documentation and samples for the `connection` and `socket` properties available on the `http.serverResponse` and `http.clientRequest` objects. PR-URL: #13617 Fixes: #12617 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>

refack · 2017-06-19T17:26:44Z

@silverwind code does not lint 😢 Probably new lint rules added in #13645

refack · 2017-06-19T17:27:23Z

@silverwind code does not lint 😢 Probably new lint rules added in #13645

P.S. we lint the JS in the snippets

silverwind · 2017-06-19T17:43:05Z

Argh, I forgot that we lint the docs.

I was able to force-push a fix as the commit was still on HEAD so this has now landed on 24ecc33.

Adds documentation and samples for the `connection` and `socket` properties available on the `http.serverResponse` and `http.clientRequest` objects. PR-URL: #13617 Fixes: #12617 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>

jasnell · 2017-06-19T17:58:22Z

Ugh. Per the collab guidelines: However, you are only allowed to force push to any Node.js branch within 10 minutes from your original push. If someone else pushes to the branch or the 10 minute period passes, consider the commit final.

This force push happened 2 hours later. Please do not do that as it causes quite a bit of hassle for other devs.

silverwind · 2017-06-19T18:35:15Z

Yes, I'm aware of that rule. Just thought it's easier to backport as one commit later. What real harm does it cause when the replaced commit is still on HEAD?

addaleax · 2017-06-19T18:59:09Z

What real harm does it cause when the replaced commit is still on HEAD?

@silverwind The question is not really where the commit is in the history; the problem is more that people who update their clones from upstream now have a bogus master after force-pushing. For example, I think new PRs based on that old master would appear to include the commits that were force-pushed out of the way.

jasnell · 2017-06-19T19:08:25Z

I have four local branches and one open PR branch that I had rebased off master when I started today that I ended up having to go back and correct after the force push. Forcing outside of the 10 minute window ends up meaning other collaborators end up having to do extra work they really shouldn't have to do.

Adds documentation and samples for the `connection` and `socket` properties available on the `http.serverResponse` and `http.clientRequest` objects. PR-URL: #13617 Fixes: #12617 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>

nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Jun 11, 2017

Trott reviewed Jun 11, 2017

View reviewed changes

Trott previously approved these changes Jun 11, 2017

View reviewed changes

lpinca reviewed Jun 12, 2017

View reviewed changes

aqrln reviewed Jun 12, 2017

View reviewed changes

cjihrig approved these changes Jun 12, 2017

View reviewed changes

aqrln approved these changes Jun 12, 2017

View reviewed changes

jasnell approved these changes Jun 13, 2017

View reviewed changes

JustinBeckwith force-pushed the 12617 branch 2 times, most recently from 0b02a9a to 3019f16 Compare June 17, 2017 21:11

addaleax reviewed Jun 17, 2017

View reviewed changes

JustinBeckwith force-pushed the 12617 branch from 3019f16 to aecaa58 Compare June 17, 2017 21:17

addaleax approved these changes Jun 18, 2017

View reviewed changes

silverwind suggested changes Jun 18, 2017

View reviewed changes

doc: document res.connection and res.socket

e854c10

Adds documentation and samples for the `connection` and `socket` properties available on the `http.serverResponse` and `http.clientRequest` objects. Fixes: nodejs#12617

JustinBeckwith force-pushed the 12617 branch from aecaa58 to e854c10 Compare June 19, 2017 06:17

silverwind approved these changes Jun 19, 2017

View reviewed changes

silverwind closed this Jun 19, 2017

addaleax mentioned this pull request Jun 24, 2017

v8.2.0 proposal #13744

Merged

MylesBorins added the dont-land-on-v6.x label Aug 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: document res.connection and res.socket #13617

doc: document res.connection and res.socket #13617

JustinBeckwith commented Jun 11, 2017 •

edited

Loading

Trott left a comment

Trott commented Jun 11, 2017

mscdex commented Jun 11, 2017

JustinBeckwith commented Jun 11, 2017

JustinBeckwith commented Jun 12, 2017 •

edited

Loading

JustinBeckwith commented Jun 12, 2017

lpinca Jun 12, 2017 •

edited

Loading

aqrln Jun 12, 2017

aqrln Jun 12, 2017

aqrln Jun 12, 2017

JustinBeckwith commented Jun 12, 2017

aqrln left a comment

aqrln Jun 12, 2017

aqrln Jun 12, 2017

jasnell left a comment

silverwind commented Jun 13, 2017

JustinBeckwith commented Jun 17, 2017

addaleax Jun 17, 2017

silverwind Jun 18, 2017 •

edited

Loading

JustinBeckwith commented Jun 19, 2017

silverwind commented Jun 19, 2017

refack commented Jun 19, 2017

refack commented Jun 19, 2017

silverwind commented Jun 19, 2017

jasnell commented Jun 19, 2017

silverwind commented Jun 19, 2017

addaleax commented Jun 19, 2017

jasnell commented Jun 19, 2017


		* `socket` {net.Socket}

		Reference to the underlying socket. `socket` is an object of type

doc: document res.connection and res.socket #13617

doc: document res.connection and res.socket #13617

Conversation

JustinBeckwith commented Jun 11, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Trott left a comment

Choose a reason for hiding this comment

Trott commented Jun 11, 2017

mscdex commented Jun 11, 2017

JustinBeckwith commented Jun 11, 2017

JustinBeckwith commented Jun 12, 2017 • edited Loading

JustinBeckwith commented Jun 12, 2017

lpinca Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

aqrln Jun 12, 2017

Choose a reason for hiding this comment

aqrln Jun 12, 2017

Choose a reason for hiding this comment

aqrln Jun 12, 2017

Choose a reason for hiding this comment

JustinBeckwith commented Jun 12, 2017

aqrln left a comment

Choose a reason for hiding this comment

aqrln Jun 12, 2017

Choose a reason for hiding this comment

aqrln Jun 12, 2017

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

silverwind commented Jun 13, 2017

JustinBeckwith commented Jun 17, 2017

addaleax Jun 17, 2017

Choose a reason for hiding this comment

silverwind Jun 18, 2017 • edited Loading

Choose a reason for hiding this comment

JustinBeckwith commented Jun 19, 2017

silverwind commented Jun 19, 2017

refack commented Jun 19, 2017

refack commented Jun 19, 2017

silverwind commented Jun 19, 2017

jasnell commented Jun 19, 2017

silverwind commented Jun 19, 2017

addaleax commented Jun 19, 2017

jasnell commented Jun 19, 2017

JustinBeckwith commented Jun 11, 2017 •

edited

Loading

JustinBeckwith commented Jun 12, 2017 •

edited

Loading

lpinca Jun 12, 2017 •

edited

Loading

silverwind Jun 18, 2017 •

edited

Loading