Clarify args accepted in docs for http.Server.listen #20142
Conversation
nodejs-github-bot
added
doc
doc/api/http.md
Outdated
| @@ -920,6 +920,13 @@ Stops the server from accepting new connections. See [`net.Server.close()`][]. | |||
| Starts the HTTP server listening for connections. | |||
| This method is identical to [`server.listen()`][] from [`net.Server`][]. | |||
|
|
|||
| Possible signatures: | |||
There was a problem hiding this comment.
I don't know if it's makes sense as it is already linked to https://nodejs.org/dist/latest/docs/api/net.html#net_server_listen which list the possible signatures. However the last one can be improved. Should we change this PR and tackle that instead?
There was a problem hiding this comment.
I see your point, maybe we could move the spec discussion to this issue #16300.
Was you suggestion to add the last signature to net.Server.listen() instead?
There was a problem hiding this comment.
Yes, exactly. I'm proposing this basically because of 278f653.
There was a problem hiding this comment.
Is that very different from the last signature in net.Server.listen()?
Perhaps the initial issue does not need to be addressed.
There was a problem hiding this comment.
Yes, it's quite different because it makes it clear which parameters will be used in each scenario. The signature in net.Server.listen makes it seem like it's possible to leave out port & host and include backlog, but it's not actually. The signature @joyeecheung came up with is more precise.
doc/api/http.md
Outdated
| @@ -920,6 +920,13 @@ Stops the server from accepting new connections. See [`net.Server.close()`][]. | |||
| Starts the HTTP server listening for connections. | |||
| This method is identical to [`server.listen()`][] from [`net.Server`][]. | |||
|
|
|||
| Possible signatures: | |||
|
|
|||
| server.listen(handle[, backlog][, callback]) | |||
There was a problem hiding this comment.
It seems this is not rendered very well, so maybe this can be formatted as a list or a code block in backticks if we decide to leave it here in this form.
There was a problem hiding this comment.
Ah, my mistake. Thanks.
musgravejw
force-pushed
the
master
branch
3 times, most recently
from
ca1e9c2
to
8518da5
Compare
|
@lpinca @apapirovski Updated commit based on discussion, does this capture the consensus? |
|
LGTM, thank you. |
doc/api/net.md
Outdated
| @@ -192,7 +192,7 @@ Possible signatures: | |||
| * [`server.listen(options[, callback])`][`server.listen(options)`] | |||
| * [`server.listen(path[, backlog][, callback])`][`server.listen(path)`] | |||
| for [IPC][] servers | |||
| * [`server.listen([port][, host][, backlog][, callback])`][`server.listen(port, host)`] | |||
| * [`server.listen([[[port[, hostname[, backlog]]][, callback])`] | |||
There was a problem hiding this comment.
The trailing [`server.listen(port, host)`] part should not be removed though.
There was a problem hiding this comment.
There is also another occurrence on line 267. Can you please also fix that?
|
My mistake, updated the commit. |
lpinca
added
the
author ready
PR-URL: #20142 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
|
Landed in 3cb8e64 Congratulations @musgravejw on your first commit to Node.js core! 🎉🎉🎉 |
|
This shouldn't have landed yet... It didn't correct the instance it's linking to. Opening a PR to fix. |
PR-URL: #20142 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Additional documentation around http.Server.listen() argument list.
Closes #16300
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes