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: fix net.Server.listen bind behavior #503

Closed
wants to merge 2 commits into from
Closed

doc: fix net.Server.listen bind behavior #503

wants to merge 2 commits into from

Conversation

zertosh
Copy link
Contributor

@zertosh zertosh commented Jan 19, 2015

2272052 changed net.Server.listen's behavior so that when the bind address is omitted, IPv6 is tried first and IPv4 is used as a fallback.

Also, added the note about the random port to http and tls, since that's true for them too but only net mentioned it. And, changed the argument name to hostname for net and tls because, although it's not named in listen, that argument is eventually passed to dns.lookup and it calls it hostname (plus http already called it that).

@rvagg
Copy link
Member

rvagg commented Jan 23, 2015

+1 from me but it'd probably be good to be explicit about what :: and 0.0.0.0 do like in the original docs and spell out that they accept connections on "any IPv6" and "any IPv4" addresses respectively.

@zertosh
Copy link
Contributor Author

zertosh commented Jan 23, 2015

@rvagg How's the wording now?

Also, this change in net didn't make it to the changelog's "Summary of changes from Node.js v0.10.35 to io.js v1.0.0" - in fact, there isn't even a net section. Should I add a quick entry? Something like "Changed net.Server.listen's behavior such that, when the bind address is omitted, IPv6 is tried first, and IPv4 is used as a fallback."

@rvagg
Copy link
Member

rvagg commented Jan 23, 2015

@zertosh yes, this looks good to me

@indutny can you confirm that this doc matches the new behaviour? (I don't really want to take sole responsibility for the doc change)

@zertosh: best to open a separate PR to change/add to the changelog, but +1 on that, we've had a few additions to that section since 1.0.0.

rvagg pushed a commit that referenced this pull request Jan 27, 2015
PR-URL: #503
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Jan 27, 2015

Landed @ 5c7ab96, in future @zertosh can you make sure you push as a single commit instead of spanning multiple commits (I think the CONTRIBUTING.md doc mentions this). Thanks!

@rvagg rvagg closed this Jan 27, 2015
@zertosh
Copy link
Contributor Author

zertosh commented Jan 27, 2015

@rvagg sorry, I meant to squash the commits. I'll make sure to keep it in mind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants