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

doc: improve server.listen() random port #7976

Closed
wants to merge 1 commit into from

Conversation

phillipj
Copy link
Member

@phillipj phillipj commented Aug 4, 2016

  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Minor rewording related to making a server listen to a random port, and added how to retrieve which port was randomly chosen by the OS. Although the previous text ".. to have the operating system assign an available port" is correct, I think using the term "random port" is easier to spot and usually what one might be searching for when looking for this type of functionality.

Also changed documented server.listen() signature as it does in fact not require port to be provided (lib/net.js#L1342).

/cc @nodejs/documentation

@phillipj phillipj added the doc Issues and PRs related to the documentations. label Aug 4, 2016
@mscdex mscdex added http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem. labels Aug 4, 2016
port value of `0` to have the operating system assign an available port.
(`::`) when IPv6 is available, or any IPv4 address (`0.0.0.0`) otherwise.
Omit the port argument to have the operating system assign a random port,
which can later be retrieved in `server.address().port`.
Copy link
Member

Choose a reason for hiding this comment

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

s/in/using

Copy link
Contributor

Choose a reason for hiding this comment

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

"later" is when? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@ronkorving hah, good catch! Went with after the 'listening' event has been emitted instead.

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

A few nits but generally LGTM! Thank you!

(`::`) when IPv6 is available, or any IPv4 address (`0.0.0.0`) otherwise. Use a
port value of `0` to have the operating system assign an available port.
(`::`) when IPv6 is available, or any IPv4 address (`0.0.0.0`) otherwise.
Omit the port argument to have the operating system assign a random port,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should completely remove the fact that 0 can be used. Could we combine the existing content and your changes?

@cjihrig
Copy link
Contributor

cjihrig commented Aug 4, 2016

LGTM with a comment.

(`::`) when IPv6 is available, or any IPv4 address (`0.0.0.0`) otherwise. Use a
port value of `0` to have the operating system assign an available port.
(`::`) when IPv6 is available, or any IPv4 address (`0.0.0.0`) otherwise.
Omit the port argument or use a port value of `0` to have the operating system
Copy link
Member

@jasnell jasnell Aug 5, 2016

Choose a reason for hiding this comment

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

Small nit: Omit the port argument, or use a port value of0, to have ...
(here and below)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@jasnell
Copy link
Member

jasnell commented Aug 5, 2016

One additional nit... otherwise LGTM!

Minor rewording related to making a server listen to a random port,
and added how to retrieve which port was randomly chosen by the OS.

Also changed documented `server.listen()` signature as it does in fact
not require `port` to be provided.

PR-URL: nodejs#7976
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@phillipj
Copy link
Member Author

phillipj commented Aug 5, 2016

@phillipj
Copy link
Member Author

phillipj commented Aug 5, 2016

CI resulted in some broken node-test-binary-arm tests which I can't imagine is related to a simple doc change, so this is good to go.

@Trott
Copy link
Member

Trott commented Aug 5, 2016

Those addon tests do extract code from docs, so they could conceivably be affected by a doc change, but I don't think the docs edited here are the ones involved.

Uh, but re-running CI again anyway: https://ci.nodejs.org/job/node-test-commit/4421/

@phillipj
Copy link
Member Author

phillipj commented Aug 5, 2016

Those addon tests do extract code from docs ..

@Trott huh, got any pointers to what that is exactly so I can read up on that for later?

@Trott
Copy link
Member

Trott commented Aug 5, 2016

tools/doc/addon-verify.js is where the magic happens. Looks like it only scrapes code from addons.md.

@jasnell
Copy link
Member

jasnell commented Aug 5, 2016

Thank you very much @phillipj. CI is green!

phillipj added a commit that referenced this pull request Aug 5, 2016
Minor rewording related to making a server listen to a random port,
and added how to retrieve which port was randomly chosen by the OS.

Also changed documented `server.listen()` signature as it does in fact
not require `port` to be provided.

PR-URL: #7976
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@phillipj
Copy link
Member Author

phillipj commented Aug 5, 2016

Landed in 66af6a9

@phillipj phillipj closed this Aug 5, 2016
@phillipj phillipj deleted the doc-server-port branch August 5, 2016 21:32
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Minor rewording related to making a server listen to a random port,
and added how to retrieve which port was randomly chosen by the OS.

Also changed documented `server.listen()` signature as it does in fact
not require `port` to be provided.

PR-URL: #7976
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
phillipj added a commit to phillipj/node that referenced this pull request Aug 22, 2016
Minor rewording related to making a server listen to a random port,
and added how to retrieve which port was randomly chosen by the OS.

Also changed documented `server.listen()` signature as it does in fact
not require `port` to be provided.

PR-URL: nodejs#7976
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@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. http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants