dns, net, internal/net: migrate to internals/error #11738

sousandrei · 2017-03-08T03:58:31Z

Assign codes to dns.js errors
Assign codes to net.js errors
Assign codes to internal/net.js errors
Added respective documentation

Error codes reused and respective PRs:
#11294

ERR_INVALID_ARGS
ERR_INVALID_ARG_TYPE
ERR_INVALID_CALLBACK

ERR_INVALID_PORT (made some alterations to it)

Semver-major because this updates specific error messages and converts errors over to use the new internal/errors.js mechanism.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

errors, net, dns, doc

mscdex · 2017-03-08T08:02:09Z

lib/internal/errors.js

+  var msg = `The "${name}" argument must be `;
+  if (Array.isArray(expected)) {
+    var len = expected.length;
+    expected = expected.map((i) => String(i));


Avoid these functional Array methods for performance.

mscdex · 2017-03-08T08:02:50Z

lib/internal/errors.js

+    msg += `. Received ${actual !== null ? typeof actual : 'null'}`;
+  }
+  return msg;
+}


Missing newline at end of file.

mscdex · 2017-03-08T08:05:12Z

doc/api/errors.md


 When present (e.g. in `net` or `dgram`), the `error.address` property is a string
 describing the address to which the connection failed.

 #### error.port

-* {Number}
+* {number}


These kinds of changes should be in a separate commit.

mscdex · 2017-03-08T08:05:43Z

doc/api/errors.md

 [online]: http://man7.org/linux/man-pages/man3/errno.3.html
 [stream-based]: stream.html
 [syscall]: http://man7.org/linux/man-pages/man2/syscall.2.html
 [try-catch]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch
 [V8's stack trace API]: https://github.com/v8/v8/wiki/Stack-Trace-API
-[vm]: vm.html
+[vm]: vm.html


What's this change?

mscdex · 2017-03-08T08:07:51Z

doc/api/errors.md

@@ -255,9 +255,16 @@ will affect any stack trace captured *after* the value has been changed.
 If set to a non-number value, or set to a negative number, stack traces will
 not capture any frames.

-### error.message
+#### error.code


Why is this being duplicated? error.code already exists below.

mscdex · 2017-03-08T08:08:51Z

lib/dns.js

  } else if (typeof options === 'function') {
    callback = options;
    family = 0;
  } else if (typeof callback !== 'function') {
-    throw new TypeError('Invalid arguments: callback must be passed');
+    // throw new TypeError('Invalid arguments: callback must be passed');


This needs to be removed.

mscdex · 2017-03-08T08:09:24Z

lib/dns.js

-      err === uv.UV_EAI_NODATA ||
-      err === uv.UV_EAI_NONAME) {
+    err === uv.UV_EAI_NODATA ||
+    err === uv.UV_EAI_NONAME) {


This should be reverted, the conditionals should be lined up.

mscdex · 2017-03-08T08:09:47Z

lib/dns.js

-        hints !== cares.AI_V4MAPPED &&
-        hints !== (cares.AI_ADDRCONFIG | cares.AI_V4MAPPED)) {
-      throw new TypeError('Invalid argument: hints must use valid flags');
+      hints !== cares.AI_ADDRCONFIG &&


Ditto about reverting the indentation changes.

mscdex · 2017-03-08T08:11:15Z

lib/internal/errors.js

 // Add new errors from here...
+
+// Errors from 111294, port error from 11302


I think 111294 here should be 11294 instead? Also it's a little bit vague about what this comment is referring to, perhaps it can just be removed?

mscdex · 2017-03-08T08:12:26Z

lib/internal/errors.js

+  var msg = `The "${name}" argument must be `;
+  if (Array.isArray(expected)) {
+    var len = expected.length;
+    expected = expected.map((i) => String(i));


Ditto about Array methods.

mscdex · 2017-03-08T08:12:59Z

lib/net.js

@@ -975,7 +975,8 @@ function lookupAndConnect(self, options) {
  }

  if (options.lookup && typeof options.lookup !== 'function')
-    throw new TypeError('"lookup" option should be a function');
+    // throw new TypeError('"lookup" option should be a function');


This needs to be removed.

sam-github · 2017-03-08T15:36:21Z

the other migrations were done module by module, perhaps this should be two PRs, dns: ... and net: ...? Either way, internals/net isn't a sub-system, its net: .

Also, the first commit doesn't follow git commit format guidelines, and the doc change has to be a different PR, its not semver-major, putting it in this PR means it won't be backported.

sousandrei · 2017-03-08T16:57:55Z

@sam-github: i was working on dns: and it broke some tests on net: so i ended up migrating that too.
should i fix in this PR the things pointed out by @mscdex in this PR or open a new one?

about the docs, i should remove from this PR and create a new one dedicated to it, right?

sam-github · 2017-03-08T17:35:06Z

@jasnell was leading the errors stuff, he should weigh in. My suggestion would be to

open an entirely new PR for the docs commit, and remove the docs commit from this PR
fix the things @mscdex pointed out
split this PR (minus the docs commit, of course) into two commits, the first one for net and the second for dns (assuming that dns depends on net, but net doesn't depend on dns)

if dns and net are so intertwined they have to be updated together, then one commit dns, net: .... would be fine instead of seperate net: ... and dns: ... commits.

jasnell · 2017-03-08T17:36:36Z

Please separate the internal errors stuff into a separate pr. I'll be tackling those next week

sousandrei · 2017-03-08T22:43:49Z

@jasnell can you clarify how should i proceed with this?
Is there a problem making a commit for dns: then one for net:, after that one for the tests and other for the doc?

i was going to separate the doc from the PR but every other PR in the migration have docs with it, so I am confused

About separating internal errors stuff in another PR, you mean internal/net? Even if it breaks the tests?

fhinkel · 2017-06-07T16:51:15Z

@sousandrei Can you rebase?

Tests should always work after in every single commit. Documentation changes should go with the relevant code changes in 1 PR. PRs that address a different problem or are a separate refactoring should be in a separate PR.

fhinkel · 2017-06-28T10:48:08Z

I'm closing this because it's been inactive for quite a while. Feel free to reopen or ping a collaborator to get it reopened if needed.

sousandrei added 3 commits March 7, 2017 02:25

dns.js errors to internal/errrors.js

3525e65

dns, net, internal/net: migrate to internals/error

07add7e

ERR_SETTING_SERVERS added

1607a19

nodejs-github-bot added dns Issues and PRs related to the dns subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. net Issues and PRs related to the net subsystem. labels Mar 8, 2017

hiroppy added the doc Issues and PRs related to the documentations. label Mar 8, 2017

mscdex reviewed Mar 8, 2017

View reviewed changes

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 8, 2017

jasnell added the blocked PRs that are blocked by other issues or PRs. label Apr 5, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

fhinkel added the stalled Issues and PRs that are stalled. label Jun 7, 2017

fhinkel closed this Jun 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns, net, internal/net: migrate to internals/error #11738

dns, net, internal/net: migrate to internals/error #11738

sousandrei commented Mar 8, 2017 •

edited

Loading

mscdex Mar 8, 2017

mscdex Mar 8, 2017

mscdex Mar 8, 2017 •

edited

Loading

mscdex Mar 8, 2017

mscdex Mar 8, 2017

mscdex Mar 8, 2017

mscdex Mar 8, 2017

mscdex Mar 8, 2017

mscdex Mar 8, 2017 •

edited

Loading

mscdex Mar 8, 2017

mscdex Mar 8, 2017

sam-github commented Mar 8, 2017

sousandrei commented Mar 8, 2017

sam-github commented Mar 8, 2017

jasnell commented Mar 8, 2017

sousandrei commented Mar 8, 2017

fhinkel commented Jun 7, 2017

fhinkel commented Jun 28, 2017

		// Add new errors from here...

		// Errors from 111294, port error from 11302

dns, net, internal/net: migrate to internals/error #11738

dns, net, internal/net: migrate to internals/error #11738

Conversation

sousandrei commented Mar 8, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Mar 8, 2017 • edited Loading

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 Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Mar 8, 2017

sousandrei commented Mar 8, 2017

sam-github commented Mar 8, 2017

jasnell commented Mar 8, 2017

sousandrei commented Mar 8, 2017

fhinkel commented Jun 7, 2017

fhinkel commented Jun 28, 2017

sousandrei commented Mar 8, 2017 •

edited

Loading

mscdex Mar 8, 2017 •

edited

Loading

mscdex Mar 8, 2017 •

edited

Loading