dns: refactor dns to more modern JavaScript #5762

benjamingr · 2016-03-17T17:07:31Z

Pull Request check-list

Affected core subsystem(s)

dns

Description of change

The current dns.js file contains some pretty outdated JavaScript practices - this pull request aims to improve the file and bring it up to the standard coding style the rest of the project has.

WIP: meanwhile feedback is appreciated.

mscdex · 2016-03-17T18:04:20Z

lib/dns.js

@@ -62,12 +62,13 @@ function makeAsync(callback) {
  return function asyncCallback() {
    if (asyncCallback.immediately) {
      // The API already returned, we can invoke the callback immediately.
-      callback.apply(null, arguments);
+      callback(...arguments);


spread is still significantly slower than apply(), even in v8 4.9, so we should probably avoid changing it for now.

mscdex · 2016-03-17T18:12:11Z

IMHO the template string additions are fine, but I'm not particularly keen on the style changes unless those kinds of changes are going to be enforced by eslint.

benjamingr · 2016-03-17T18:27:44Z

@mscdex can you please be more specific about which style changes?

mscdex · 2016-03-17T18:55:09Z

lib/dns.js

@@ -192,10 +195,10 @@ 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}`);


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

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.

mscdex · 2016-03-17T18:57:02Z

lib/dns.js

@@ -38,6 +37,9 @@ function errnoException(err, syscall, hostname) {
  return ex;
 }

+function validIpVersion(isIpResult) {
+  return isIpResult !== 0;


IMHO I don't think it's necessary to extract a conditional like this into its own function.

@mscdex I would agree- but then it would certainly need a comment to explain it. I would not think "validIpVersion" from isIpResult !== 0

I'm open to suggestions here - it wasn't clear to me when I read this file what isIp returns and I had to read the C++ to realize that it returns 4 for ipv4 6 for ipv6 and 0 for "parsing failed".

The goal was to make the fact that 0 is used for "false" here explicit. I don't mind inlining those checked if you'd like but I think that this helps clarify things - I'm open to other approaches too - what do you think?

The documentation for net.isIP() describes the valid return values (including 0), so it shouldn't be private knowledge.

That's a good point, I still think it helps the code quality but if there is no consensus on that fact I'll do !== 0 since it's less objectionable and more like the old behavior.

mscdex · 2016-03-17T18:58:56Z

@benjamingr For example, the return and purely brace changes.

benjamingr · 2016-03-17T19:04:28Z

@mscdex do we have a clera coding style guide of where to put braces and where not to and where to drop lines and where not to?

I was trying to make this file consistent while I'm working on it - I don't mind any using any other coding style or reverting any changes as long as it's consistent.

mscdex · 2016-03-17T19:07:01Z

@benjamingr What I meant was unless we configure eslint to enforce those kinds of changes everywhere in node core, making those changes just in this one file just adds unnecessary noise.

Right now it's just a use whatever style you want when you're adding/changing a particular piece of code for other reasons.

williamkapke · 2016-03-17T19:07:04Z

lib/dns.js

  }
 };


-exports.getServers = function() {
+exports.getServers = () => {
  return cares.getServers();
 };


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

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

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 ?

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.

benjamingr · 2016-03-17T19:10:27Z

@mscdex the current state is that lots of files are inconsistent, I want to make things more consistent - even if not at the project level at least within the file. If you think it's not worth it I won't put any more time into it - note that it's not a particularly big part of what this PR is about.

silverwind · 2016-03-17T19:16:15Z

@benjamingr I'd suggest going on a per-rule basis instead of a per-file basis, starting with rules that don't require a huge amount of changes. Making changes without backing them up through a linter rule is going to lead to inconsistency in the long run.

As for a style guide, we don't have one but I suggest we may take a few hints from this one, which looks widely accepted.

benjamingr · 2016-03-17T19:19:32Z

Most of the braces changes here refactor for loops like:

      for (var i = 1, a = 0; a < arguments.length; ++i, ++a) 
        args[i] = arguments[a];

Into

      for (var i = 1, a = 0; a < arguments.length; ++i, ++a) {
        args[i] = arguments[a];
      }

Which I find more readable. If either of you (or anyone else) finds it objectionable I'll revert it but I personally find the former confusing.

r-52 · 2016-03-17T19:27:50Z

lib/dns.js

+  if (family !== 0 && family !== 4 && family !== 6) {
+    const msg = `Invalid argument: family must be 4 or 6, got {family}`;
+    throw new TypeError(msg);
+  }


Looks like the $ for the template string went missing.

Trott · 2016-03-18T04:30:08Z

Why the semver-major label? /cc @vkurchatkin

Trott · 2016-03-18T05:12:29Z

@mscdex wrote:

IMHO the template string additions are fine, but I'm not particularly keen on the style changes unless those kinds of changes are going to be enforced by eslint.

Don't want to go too far on a tangent here, but if you would go so far as to support lint-enforcement of template literals over string concatenation (or if you would object to such a thing), please take a look at #5778

mscdex · 2016-03-18T05:43:27Z

lib/dns.js


-  port = +port;
+  port = Number(port);


Why this change?

Well, unary + and Number do the same thing but Number is more explicit and obvious. Not everyone is familiar with things like this so when there is no doubt about performance issues I'd rather refactor to cleaner code.

I'm also not sure about the >>>s to convert to integer numbers. We use them for the options object in this file and I wonder if I shouldn't convert them to Math.floor since it's more obvious and it's not a performance sensitive area.

r-52 · 2016-03-18T05:45:42Z

@Trott there are instances where the error message changed. (Maybe these changes could be in a separate PR?)

vkurchatkin · 2016-03-18T06:38:30Z

@Trott exporting arrow fns is semver-major

jasnell · 2016-03-18T17:32:11Z

lib/dns.js

        args[i] = arguments[a];
+      }


nit: not in love with the style changes

jasnell · 2016-03-18T17:35:26Z

Also, changing error messages is also always semver-major

jasnell · 2016-03-18T17:36:20Z

I'm +0 on this. I'm all for modernizing the code where it makes sense but the style changes just seem unnecessary.

benjamingr · 2016-03-19T12:34:04Z

Would the other collaborators prefer a different PR that modernizes the code base a little (.forEach -> .map, better errors, const, better variable names, etc) without the style changes?

I don't feel very strongly about the style changes - but I do want to fix the inconsistencies (like isIp coming from three different places, and inconsistent usage).

I think I'm pretty close to done minus final fixes. Would love to have consensus first.

Trott · 2016-03-19T17:55:52Z

My preferred approach (and the one I think I have more-or-less stuck to) is:

If the changes are within a single file so that the file is consistent with itself, that should be its own PR
Stylistic changes for which there may or may not be consensus should be done isolated from other changes in one or more than one PR. Any given PR should only have one type of stylistic change. For example, right now, I'm exploring the possibility of enabling a lint rule to enforce use of template literals instead of string concatenation. I'm not sure if such a thing would have consensus or not. If it was feasible to do change all the instances of string concatenation to template literals in a single PR, I would do that. But that would be over a thousand lines changed, so instead, I did all the instances in src and submitted a PR for that. If that lands, I'll do one for tools and one for lib etc. until everything is done and I can then submit a PR to enable the lint rules everywhere.

In contrast, putting several different stylistic changes in a single file and having that be the unit of PR is likely to be stalled by bike-shedding. There are exceptions though. Changing var to const where applicable is usually uncontroversial, for example.

benjamingr · 2016-03-22T09:59:55Z

Closing this, as it has been superceded incrementally (minus the style changes) into:

#5803
#5804
#5809

benjamingr added 3 commits March 17, 2016 18:58

initial work - normalize if/else style

8d6c22e

more template strings and less abused returns

f5c4f55

block over for loop, spread arguments from apply

2784847

benjamingr added dns Issues and PRs related to the dns subsystem. dont-land-on-v5.x labels Mar 17, 2016

mscdex reviewed Mar 17, 2016
View reviewed changes

benjamingr added 2 commits March 17, 2016 20:25

address comments

48fde13

address comments from mscdex

bfe0f40

benjamingr added 3 commits March 17, 2016 20:49

refactor forEach to map

2d54ae9

var to const

85f9b87

removed references to net module

457de39

mscdex reviewed Mar 17, 2016
View reviewed changes

changed {} to Object.create(null) to prevent __proto__ meddling

4f124f7

mscdex reviewed Mar 17, 2016
View reviewed changes

williamkapke reviewed Mar 17, 2016
View reviewed changes

vkurchatkin added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 17, 2016

remove isValidIp and check for 0 instead

6fe93ed

r-52 reviewed Mar 17, 2016
View reviewed changes

silly mistake missing $

b3ced41

Trott mentioned this pull request Mar 18, 2016

src,tools: use template literals #5778

Closed

4 tasks

mscdex reviewed Mar 18, 2016
View reviewed changes

benjamingr removed dont-land-on-v4.x labels Mar 18, 2016

jasnell reviewed Mar 18, 2016
View reviewed changes

lib/dns.js

args[i] = arguments[a];

}

Copy link

Member

jasnell Mar 18, 2016

Choose a reason for hiding this comment

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

nit: not in love with the style changes

jasnell added the error-handling label Mar 18, 2016

This was referenced Mar 19, 2016

dns: Refactor forEach to map #5803

Closed

dns: use isIp consistently #5804

Closed

benjamingr closed this Mar 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: refactor dns to more modern JavaScript #5762

dns: refactor dns to more modern JavaScript #5762

benjamingr commented Mar 17, 2016

mscdex Mar 17, 2016

mscdex commented Mar 17, 2016

benjamingr commented Mar 17, 2016

mscdex Mar 17, 2016

benjamingr Mar 17, 2016

mscdex Mar 17, 2016

williamkapke Mar 17, 2016

benjamingr Mar 17, 2016

mscdex Mar 17, 2016

benjamingr Mar 17, 2016

mscdex commented Mar 17, 2016

benjamingr commented Mar 17, 2016

mscdex commented Mar 17, 2016

williamkapke Mar 17, 2016

benjamingr Mar 17, 2016

thefourtheye Mar 17, 2016

benjamingr commented Mar 17, 2016

silverwind commented Mar 17, 2016

benjamingr commented Mar 17, 2016

r-52 Mar 17, 2016

benjamingr Mar 17, 2016

Trott commented Mar 18, 2016

Trott commented Mar 18, 2016

mscdex Mar 18, 2016

benjamingr Mar 18, 2016

r-52 commented Mar 18, 2016

vkurchatkin commented Mar 18, 2016

jasnell Mar 18, 2016

jasnell commented Mar 18, 2016

jasnell commented Mar 18, 2016

benjamingr commented Mar 19, 2016

Trott commented Mar 19, 2016

benjamingr commented Mar 22, 2016

dns: refactor dns to more modern JavaScript #5762

dns: refactor dns to more modern JavaScript #5762

Conversation

benjamingr commented Mar 17, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

mscdex commented Mar 17, 2016

benjamingr commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Mar 17, 2016

benjamingr commented Mar 17, 2016

mscdex commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Mar 17, 2016

silverwind commented Mar 17, 2016

benjamingr commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Mar 18, 2016

Trott commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-52 commented Mar 18, 2016

vkurchatkin commented Mar 18, 2016

Choose a reason for hiding this comment

jasnell commented Mar 18, 2016

jasnell commented Mar 18, 2016

benjamingr commented Mar 19, 2016

Trott commented Mar 19, 2016

benjamingr commented Mar 22, 2016