Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add declarations for net.isIP* #203

Merged
merged 2 commits into from
Jan 5, 2015
Merged

Conversation

rsolomo
Copy link
Contributor

@rsolomo rsolomo commented Dec 24, 2014

No description provided.

@@ -427,7 +427,9 @@ declare module "https" {
}

declare module "net" {
// TODO
declare function isIP(input: any): number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the input type be string? The d.ts seems to use string, and the spec is somewhat unclear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would lean towards any since the doc seems to be unclear if the input must be a string, and any would more closely match the signature of other is* functions.

Also, it's not mentioned in the documentation, but it may (or may not) be worth noting the function does have the behaivor of casting the input argument to a string.
In other words this prints true, even though the argument is not a string:

var net = require('net')
var x = {
  toString: function() {
    return '127.0.0.1'
  }
}

console.log(net.isIPv4(x)) // true

@gabelevi
Copy link
Contributor

If these functions are implicitly cast to strings, then I would recommend making the parameter type string. This means the user of the function would have to explicitly cast the argument to string before calling the function, like net.isIPv4(String(x)). My intuition is telling me

  1. I'd guess that it's somewhat rare to pass stringable objects to these APIs
  2. I'd guess actually type errors like passing null or undefined to these functions are somewhat common
  3. The workaround (casting to String) is easy enough that even if people do use stringable objects they're not terribly inconvenienced

That being said, I don't feel terribly strongly here. If you still disagree let me know and I'll merge your PR :)

@rsolomo
Copy link
Contributor Author

rsolomo commented Dec 26, 2014

Okay, that's fine. String it is.

gabelevi added a commit that referenced this pull request Jan 5, 2015
add declarations for net.isIP*
@gabelevi gabelevi merged commit 240c996 into facebook:master Jan 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants