cb may be undefined as well

[vincent-tr]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included

Affected core subsystem(s)

net (Socket.protype.connect)

[cjihrig]

I think the best approach is probably to check if typeof cb === 'function'.

[Fishrock123]

I was also going to suggest that. Yes.

[vincent-tr]

I think we need to check if cb is undefined, to respect API contract: net.connect(options[, connectListener])

I don't think that net.connect(options, 42) or net.connect(options, 'anything')
should silently ignore the second parameter without failure, what do you think ?

[joyeecheung]

Duplicate of #11762?

[joyeecheung]

BTW: normalizeArgs is supposed to always normalize the callback to null if it's not defined.

[Trott]

If we do go with this, it needs a test I think.

[Trott]

@vincent-tr Thanks for taking the time to submit this PR!

@vincent-tr and @joyeecheung: Since #11762 landed, does that mean this PR obsolete should be closed?

[cjihrig]

Since #11762 landed, does that mean this PR obsolete should be closed?

I think changing to an explicit typeof === 'function' check is still the correct thing to do. It's future proof, in case normalizeArgs() changes.

[joyeecheung]

@Trott #11762 also added a test, I don't think there are more tests this PR can add if it's only to make sure cb can be undefined (because now it's always normalized, so when it gets to this point cb can not be undefined anymore).

If it changes to typeof === 'function' as @cjihrig suggests then this is still worth merging, IMO. But I still can't see any more tests this PR need..

[vincent-tr]

@Trott @joyeecheung having a look at #11762 it handles indeed this case.
But I still think net.connect(options, 42) ornet.connect(options, 'anything') should fail instead of ignore the second parameter.

[joyeecheung]

@vincent-tr I agree letting things like net.connect(options, 42) pass is a bit weird, but I suspect there are people doing .apply or .call(...args) on it/server.listen() without much checking, so if we enforce these signatures the breakage could be bigger than expected..right now any arguments before cb in (options, cb), (port[, host], cb), (path, cb), or placed as the last one but are not functions, are ignored.

/~https://github.com/search?l=JavaScript&q=net.connect.apply&type=Code&utf8=%E2%9C%93
/~https://github.com/search?l=JavaScript&q=server.listen.apply&type=Code&utf8=%E2%9C%93

[vincent-tr]

True. Closing then, Thanks :)