doc: various improvements to net.md by joyeecheung · Pull Request #11636 · nodejs/node

joyeecheung · 2017-03-01T16:00:34Z

Improve general description of the module, specifically,
explain that it provides TCP or local communications
(domain sockets on UNIX, named pipes on Windows) functionalities.
Improve explanation of allowHalfOpen
Nest the overloaded server.listen() API in a list, explain
the common arguments and notes in the same place.

Some minor improvements:

Add description to the net.Server constructor
Add type annotations to server.listen()
Add contexts to method links

TODO: do the same thing tonet.Socket if this one is good to go.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

net, doc

Fishrock123 · 2017-03-01T16:07:05Z

doc/api/net.md

I think it also underlies UDP? or does that reimplement sockets?

UDP is different code, no support for it in net

:S just got enlightened by @addaleax , the stuff in this module is stream-based, UDP(dgram) is not.

jasnell

LGTM with a nit.

jasnell · 2017-03-01T16:06:10Z

doc/api/net.md

Please line wrap at 80 chars

addaleax

Just skimmed over it but looks pretty good in general (and helpful!) :)

addaleax · 2017-03-01T16:15:29Z

doc/api/net.md

Maybe mention that this flag is explained in socket(7)?

I trried http://man7.org/linux/man-pages/man7/socket.7.html but couldn't find an ID in that page(and it's a really long page)...maybe just add it because it's better than nothing? :3

@joyeecheung Yeah, it’s better than nothing. I would even be okay with telling people to Ctrl+F it. 😄

addaleax · 2017-03-01T16:18:04Z

doc/api/net.md

nit: Maybe use arbitrary unused port instead of random port? There doesn’t need to be any true randomness in the port choice

joyeecheung · 2017-03-01T17:14:34Z

@jasnell @addaleax comments addressed, PTAL

sam-github · 2017-03-01T17:16:42Z

doc/api/net.md

wrap to 80 columns

Fixed, thanks!

sam-github

you are almost sorted in the markdown links at the bottom of page, but otherwise, LGTM

sam-github · 2017-03-01T17:35:20Z

doc/api/net.md

this one is out of sort order now

sam-github · 2017-03-01T17:35:42Z

doc/api/net.md

out of sort order

addaleax

Awesome, LGTM!

addaleax · 2017-03-01T17:33:43Z

doc/api/net.md

Hmm… both ends of a connection are net.Sockets, so maybe servers (…) and connections (…) is a bit better?

Most of the examples for net.Socket call the returned value client though...? I guess saying it provides clients would help people looking for one.

The argument for the 'connection' listener is a net.Socket, that’s where the non-client sockets come from.

I guess saying it provides clients would help people looking for one.

Not disagreeing! It’s definitely good to state that net provides servers and clients… I’m just trying to say that I would prefer to avoid any confusion of associating net.Socket with client.

If you think this is a non-issue, feel free to ignore this :)

Perhaps

..and clients ([`net.Socket`][] calling [`connect()`][])

?

@joyeecheung In that case we could just refer to createConnection 😄

But, yeah, this is getting really bike-shed-y and I trust your judgement on this :)

I'll just put net.createServer and net.createConnection then (the classes are not usually newed anyway :D)

On a side note, what is the preferred way to create a client? net.createConnection and net.connect are just aliases now..(net.createConnection does look better when you put it alongside net.createServer though)

addaleax · 2017-03-01T17:34:23Z

doc/api/net.md

nit: missing space before the opening parenthesis

addaleax · 2017-03-01T17:36:49Z

doc/api/net.md

sends a FIN packet, thus ending the readable side of the socket.?

addaleax · 2017-03-01T17:36:51Z

doc/api/net.md

nit: its writable side

* Improve general description of the module, specifically, explain that it provides TCP or local communications (domain sockets on UNIX, named pipes on Windows) functionalities. * Improve explanation of `allowHalfOpen` * Nest the overloaded `server.listen()` API in a list, explain the common arguments and notes in the same place. Some minor improvements: * Add description to the `net.Server` constructor * Add type annotations to `server.listen()` * Add contexts to method links

joyeecheung · 2017-03-03T03:21:47Z

Addressed comments and rebased since #11167 is merged.

joyeecheung · 2017-03-04T06:27:48Z

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/6699/

joyeecheung · 2017-03-04T08:48:34Z

osx build failure is unrelated, tried another one and it build scucessfully.. https://ci.nodejs.org/job/node-test-commit-osx/8200/

Landed in 3b05153, thanks!

* Improve general description of the module, specifically, explain that it provides TCP or local communications (domain sockets on UNIX, named pipes on Windows) functionalities. * Improve explanation of `allowHalfOpen` * Nest the overloaded `server.listen()` API in a list, explain the common arguments and notes in the same place. Some minor improvements: * Add description to the `net.Server` constructor * Add type annotations to `server.listen()` * Add contexts to method links PR-URL: #11636 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

evanlucas · 2017-03-07T12:02:04Z

This is not landing cleanly on v7.x-staging. Mind submitting a backport PR?

gibfahn · 2017-06-17T20:26:29Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

nodejs-github-bot added doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem. labels Mar 1, 2017

Fishrock123 reviewed Mar 1, 2017

View reviewed changes

jasnell approved these changes Mar 1, 2017

View reviewed changes

doc/api/net.md Outdated

Copy link

Member

jasnell Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please line wrap at 80 chars

addaleax reviewed Mar 1, 2017

View reviewed changes

joyeecheung force-pushed the improve-net-doc branch 2 times, most recently from 348944d to 186a790 Compare March 1, 2017 17:16

sam-github reviewed Mar 1, 2017

View reviewed changes

sam-github approved these changes Mar 1, 2017

View reviewed changes

addaleax approved these changes Mar 1, 2017

View reviewed changes

joyeecheung added 5 commits March 3, 2017 11:11

add stream-based

d1ea3d2

socket(7), wrap links, arbitrary port

5896e3e

fix line wrap

5102522

fix link sort order

b78b5f3

joyeecheung force-pushed the improve-net-doc branch from 434b6ac to fb61a63 Compare March 3, 2017 03:20

joyeecheung added 2 commits March 3, 2017 11:24

address comments

fb61a63

fix wrap

3686d75

joyeecheung closed this Mar 4, 2017

joyeecheung mentioned this pull request Mar 5, 2017

doc: improve docs for net.connect, net.createConnection, net.Socket #11700

Closed

3 tasks

evanlucas added the dont-land-on-v7.x label Mar 7, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the backport-requested-v6.x label Jun 17, 2017

joyeecheung self-assigned this Oct 17, 2017

joyeecheung added dont-land-on-v6.x and removed backport-requested-v6.x labels Nov 1, 2017

joyeecheung removed their assignment Nov 1, 2017

joyeecheung added dont-land-on-v6.x and removed dont-land-on-v6.x labels Nov 1, 2017

Uh oh!

Conversation

joyeecheung commented Mar 1, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Mar 3, 2017

Uh oh!

joyeecheung commented Mar 4, 2017

Uh oh!

joyeecheung commented Mar 4, 2017

Uh oh!

evanlucas commented Mar 7, 2017

Uh oh!

gibfahn commented Jun 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

joyeecheung Mar 1, 2017 •

edited

Loading