Docs on http.Server.listen() need clarifications on accepted args

Version: v4-v8
Platform: macOS 10.12.6
Subsystem: http, net

This is in relation to facebook/flow#1684

While Flow typing a module that uses http.Server.listen(), I received an error for the idiomatic form widely used in Node projects:

const server = http.createServer();
server.listen(8080, function listening() {});

Flow declares the types of http.Server.listen() like this:

declare module "http" {
  declare class Server extends net$Server {
    listen(port: number, hostname?: string, backlog?: number, callback?: Function): Server;
    listen(path: string, callback?: Function): Server;
    listen(handle: Object, callback?: Function): Server;

which adheres to the documentation of the http module.

Since the net module normalizes the arguments and Node itself uses the shorthand forms in the docs on the same page (Event: 'connect', Event: 'upgrade'), I suggest to amend the documentation explaining the shorthand forms are both supported:

listen(port, callback)
listen(port, hostname, callback)

Thoughts?

I could be misunderstanding but doesn't the http module already document this as such in server.listen([port][, hostname][, backlog][, callback])? Meaning any of these arguments can be safely omitted.

@apapirovski

@apapirovski is right, any arguments inside brackets in docs are indicating optional parameters and server.listen() already shows that, so I don't think anything needs to be changed on our end. It seems like a bug/issue with Flow.

@mscdex

@mscdex @apapirovski thanks for clearing my misunderstanding, effectively the behavior of Flow is inconsistent with what server.listen() actually accepts, forcing the implementor to pass literal undefined in place of the omitted argument(s).

I'll bring it back to the Flow team for discussion.

Closing because the documentation is clear enough.

@joyeecheung

@joyeecheung I'd ask you to leave this open as I believe there are still some inconsistencies.

The docs don't specify what is the expected behavior when encountering the signature with one argument server.listen(Number), or with two arguments, server.listen(Number, Function):

// This is interpreted as a port number
server.listen(8080);
// This is interpreted as a port number
server.listen(8080, function callback() {});

which consider the first Number as port, while the notation server.listen([port][, hostname][, backlog][, callback]) suggests that you could pass a backlog but no port.

Additionally, the signature server.listen(String, Function) can match both server.listen(path[, callback]) and server.listen([port][, hostname][, backlog][, callback]) with only hostname and callback set.

I would like to clarify the expected behavior 😄

while the notation server.listen([port][, hostname][, backlog][, callback]) suggests that you could pass a backlog but no port.

I would interprete this as "if there is any argument, and the first one is a number ,then the first one is a port" though, I am not sure if this optional argument syntax has any formal standard, but to me the previous arguments can't be optional like that (server.listen(backlog[, callback]) does not match server.listen([port][, hostname][, backlog][, callback]) because the [, part is there for a reason.)

Additionally, the signature server.listen(String, Function) can match both server.listen(path[, callback]) and server.listen([port][, hostname][, backlog][, callback]) with only hostname and callback set.

I can't really see how the second one can be matched either, for the same reason above.

@claudiopro

@claudiopro OK I think there is another way to make it clearer, although that requires a lot of brackets...

server.listen([[[port[, hostname[, backlog]]][, callback])

[PR] Adds tests for Node http(s) modules, improves server.listen() defs

@joyeecheung

If someone wants to, this could be updated based on the latest feedback from @joyeecheung or instead have a little paragraph that outlines the possible combinations or something. Either way, it's probably a good idea to update with something that's more accurate.

@apapirovski

Hi, @apapirovski. After reviewing the previous comments of this "good first issue" issue, I'm happy to pick up this one and give it a "try" on your reference to the latest feedback from @ joyeecheung with suggestion in your last comment:

@ claudiopro OK I think there is another way to make it clearer, although that requires a lot of brackets...

server.listen([[[port[, hostname[, backlog]]][, callback])

Saw your last comment from 14 days ago. So just want to confirm with you again, before I get the local stuff started.

I can at least try to start with the proposed signature, learn the proposed parameters with the brackets' syntax, and use my best judgement to write a draft of possible combinations of the signature's parameters, to at least get the draft & PR started.

@BeniCheni

@BeniCheni This has actually been addressed by the PR above linked above your comment. Thanks for your interest though! Hope to see you around on another "good first issue". 👍

@apapirovski

Thanks, @apapirovski. Ah missed the referred PR above my comment. My Bad. 🤗 Will poke around on there “good first time” or “help wanted” labeled open issues. Have a great day!

Navigation Menu

Repository navigation

Docs on http.Server.listen() need clarifications on accepted args #16300

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Docs on http.Server.listen() need clarifications on accepted args #16300

Description

Activity

apapirovski commented on Oct 18, 2017

mscdex commented on Oct 18, 2017

claudiopro commented on Oct 19, 2017

joyeecheung commented on Oct 19, 2017

claudiopro commented on Oct 19, 2017

joyeecheung commented on Oct 19, 2017

joyeecheung commented on Oct 19, 2017

apapirovski commented on Apr 13, 2018

BeniCheni commented on Apr 27, 2018

apapirovski commented on Apr 27, 2018

BeniCheni commented on Apr 27, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions