net: type check createServer options object

[sam-github]

net.createServer('aPipe') and net.createServer(8080) are mistakes, and
now throw a TypeError instead of silently being treated as an object.

[thefourtheye]

Use const

[cjihrig]

Space between if and (

[Trott]

Nit: typeof null returns 'object' so you can skip the check for options == null unless the intention is for the not-strict equality to allow undefined, in which case I imagine checking for undefined would be more clear. Meh, maybe not so much. == null is pretty idiomatic for "null or undefined".

[cjihrig]

LGTM

[evanlucas]

Is this semver-major with the throw or are we considering this a bugfix?

[cjihrig]

Seems like a bug fix to me. Currently, if you passed something like 5 as options, you'd hit options = options || {};, and then later crash on this.allowHalfOpen = options.allowHalfOpen || false;.

[evanlucas]

Ah that makes sense

[evanlucas]

So I guess the behavior would still be changing if false were passed?

[cjihrig]

Yea I guess so. Same for NaN, as undocumented as it may be :-)

[evanlucas]

LGTM minus the nit that @cjihrig pointed out

[benjamingr]

Anything blocking this?

[cjihrig]

@benjamingr just one style nit, and a CI run, as far as I can tell. If you want, you can address the style nit and land it, assuming CI is happy.

[whitlockjc]

We also might want to squash the commits no?

[cjihrig]

Yes

[benjamingr]

Sure, I'll shape it up and land it tomorrow

[benjamingr]

CI: https://ci.nodejs.org/job/node-test-pull-request/1925/

[benjamingr]

The linter is angry for missing a space between the if and a ( - otherwise tests pass - I'll fix this locally.

[benjamingr]

Landed in a78b334 . Thanks.

[jasnell]

Marked as a semver-major because it adds a new throw.

[Fishrock123]

@evanlucas make sure you catch this for v5.8.1 ^

[evanlucas]

@Fishrock123 semver-major though?

[benjamingr]

See the discussion above @jasnell @Fishrock123 :

Seems like a bug fix to me. Currently, if you passed something like 5 as options, you'd hit options = options || {};, and then later crash on this.allowHalfOpen = options.allowHalfOpen || false;.

[jasnell]

hmm... not sure about that. If I passed 5 as options, options = options || {} would mean options = 5, which means this.allowHalfOpen = options.allowHalfOpen || false would not crash, it would simply resolve to this.allowHalfOpen = false.

[benjamingr]

That's right, I tend to agree.