Skip to content

dns: refactor dns to more modern JavaScript #5762

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.

Sign up for GitHub

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

Closed
benjamingr wants to merge 11 commits into from
Closed

dns: refactor dns to more modern JavaScript #5762

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8d6c22e
initial work - normalize if/else style
benjamingr Mar 17, 2016
f5c4f55
more template strings and less abused returns
benjamingr Mar 17, 2016
2784847
block over for loop, spread arguments from apply
benjamingr Mar 17, 2016
48fde13
address comments
benjamingr Mar 17, 2016
bfe0f40
address comments from mscdex
benjamingr Mar 17, 2016
2d54ae9
refactor forEach to map
benjamingr Mar 17, 2016
85f9b87
var to const
benjamingr Mar 17, 2016
457de39
removed references to net module
benjamingr Mar 17, 2016
4f124f7
changed {} to Object.create(null) to prevent __proto__ meddling
benjamingr Mar 17, 2016
6fe93ed
remove isValidIp and check for 0 instead
benjamingr Mar 17, 2016
b3ced41
silly mistake missing $
benjamingr Mar 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
address comments
  • Loading branch information
@benjamingr
benjamingr committed Mar 17, 2016
commit 48fde13751ad777302751495d89791bbbfb2380f
22 changes: 12 additions & 10 deletions lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function makeAsync(callback) {
return function asyncCallback() {
if (asyncCallback.immediately) {
// The API already returned, we can invoke the callback immediately.
callback(...arguments);
callback.apply(null, arguments);
} else {
var args = new Array(arguments.length + 1);
args[0] = callback;
Expand Down Expand Up @@ -96,12 +96,12 @@ function onlookupall(err, addresses) {
}

const results = Array(addresses.length);
Copy link
Copy Markdown
Contributor

@mscdex mscdex Mar 17, 2016

Choose a reason for hiding this comment

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

For array instantiation like this new should be used. Also, results.push() below will need to be changed too.

Copy link
Copy Markdown
Member Author

@benjamingr benjamingr Mar 17, 2016

Choose a reason for hiding this comment

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

Thanks


for (var i = 0; i < addresses.length; i++) {
results.push({
results[i] = {
address: addresses[i],
family: this.family || (addresses[i].indexOf(':') >= 0 ? 6 : 4)
});
};
}

this.callback(null, results);
Expand Down Expand Up @@ -182,7 +182,8 @@ exports.lookup = function lookup(hostname, options, callback) {

function onlookupservice(err, host, service) {
if (err) {
return this.callback(errnoException(err, 'getnameinfo', this.host));
this.callback(errnoException(err, 'getnameinfo', this.host));
return;
}

this.callback(null, host, service);
Expand All @@ -192,7 +193,7 @@ function onlookupservice(err, host, service) {
// lookupService(address, port, callback)
exports.lookupService = function(host, port, callback) {
if (arguments.length !== 3) {
throw new Error('Invalid arguments');
throw new TypeError(`Expected 3 arguments, got ${arguments.length}`);
Copy link
Copy Markdown
Contributor

@mscdex mscdex Mar 17, 2016

Choose a reason for hiding this comment

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

This isn't really a TypeError per se, since it is more to do with argument count rather than argument type.

Copy link
Copy Markdown
Member Author

@benjamingr benjamingr Mar 17, 2016

Choose a reason for hiding this comment

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

Well, the type of lookupService is a function that takes three arguments and returns its results - if you pass an incorrect number of arguments to it then you're misusing the type.

For example - when you call setTimeout with 0 arguments in browsers you get a TypeError. When you call Array#reduce with 1 argument on an empty array you get a TypeError about the missing argument, and so on.

I don't feel very strongly about this - but the language and most APIs I know throw type errors in this case.

}

if (cares.isIP(host) === 0) {
Expand Down Expand Up @@ -236,9 +237,10 @@ function resolver(bindingName) {

return function query(name, callback) {
if (typeof name !== 'string') {
throw new Error('"name" argument must be a string');
const msg = `"name" argument must be a string, got ${typeof name}`;
throw new TypeError(msg);
} else if (typeof callback !== 'function') {
throw new Error('"callback" argument must be a function');
throw new TypeError('"callback" argument must be a function');
}

callback = makeAsync(callback);
Expand Down Expand Up @@ -272,7 +274,7 @@ exports.resolveSoa = resolveMap.SOA = resolver('querySoa');
exports.reverse = resolver('getHostByAddr');


exports.resolve = function(hostname, type_, callback_) {
exports.resolve = (hostname, type_, callback_) => {
var resolver, callback;
if (typeof type_ === 'string') {
resolver = resolveMap[type_];
Expand All @@ -292,7 +294,7 @@ exports.resolve = function(hostname, type_, callback_) {
};


exports.getServers = function() {
exports.getServers = () => {
return cares.getServers();
};
Copy link
Copy Markdown
Contributor

@williamkapke williamkapke Mar 17, 2016

Choose a reason for hiding this comment

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

exports.getServers = () => cares.getServers();

Are the curlys required by coding convention here? ::shrugs::

Copy link
Copy Markdown
Member Author

@benjamingr benjamingr Mar 17, 2016

Choose a reason for hiding this comment

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

I think they're required in multi-line functions. I recall talking about it but I'm not sure what conclusion was reached and I'm fine with either - @thefourtheye ?

Copy link
Copy Markdown
Contributor

@thefourtheye thefourtheye Mar 17, 2016

Choose a reason for hiding this comment

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

I remember the discussion regarding the parens in arrow function parameters, but not sure about the body. I'll have to look. But I feel that having curly braces for even a single expression kinda beats the purpose of arrow functions.


Expand Down