net: type check createServer options object by sam-github · Pull Request #2904 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation24 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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

sam-github added the net

Issues and PRs related to the net subsystem.

label

Sep 16, 2015

thefourtheye

@@ -0,0 +1,7 @@
'use strict';
var common = require('../common');
var assert = require('assert');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use const

cjihrig

@@ -1080,12 +1080,14 @@ function Server(options, connectionListener) {
connectionListener = options;
options = {};
self.on('connection', connectionListener);
} else {
} else if(options == null \|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space between if and (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

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

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;.

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

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

LGTM minus the nit that @cjihrig pointed out

@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.

We also might want to squash the commits no?

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

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

benjamingr pushed a commit that referenced this pull request

Mar 15, 2016

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

PR-URL: #2904 Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Evan Lucas evanlucas@me.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

jasnell added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Mar 15, 2016

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

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;.

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.

That's right, I tend to agree.

jasnell added a commit that referenced this pull request

Apr 26, 2016