https: use internal/errors.js #11293

seppevs · 2017-02-10T16:52:01Z

Change https.js so it makes use of the new internal/errors.js module.

See #11273 for more info.

cc @jasnell

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

https

jasnell

Thank you for jumping in on this! I really do appreciate it!

There are still a couple of things that need to be done on this. Please take a look at my PR #11294 as an example. Documentation for the error code should be added to the end of doc/api/errors.md the same way I do so in #11294. Feel free to duplicate some of the boilerplate there and we'll work out the conflicts as we review and land PRs.

jasnell · 2017-02-10T17:09:57Z

lib/https.js

@@ -195,7 +196,7 @@ exports.request = function request(options, cb) {
  if (typeof options === 'string') {
    options = url.parse(options);
    if (!options.hostname) {
-      throw new Error('Unable to determine the domain name');
+      throw new errors.Error('UNABLE_TO_DETERMINE_DOMAIN_NAME');


Please use something shorter with the ERR_ prefix. E.g. ERR_NO_DOMAIN

jasnell · 2017-02-10T17:14:02Z

Marking as semver-major defensively. Semver-minor may be ok once we're sure nothing breaks.

seppevs · 2017-02-10T19:12:12Z

@jasnell Thank you for the clarifications. I amended the changes you requested to my previous commit.

joyeecheung · 2017-02-10T19:18:13Z

doc/api/errors.md

@@ -255,6 +255,13 @@ 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.code


I am not sure whether this section belongs here...at least we need to mention that this property doesn't appear in all Errors, and it is set by Node.js?

@seppevs ... yeah, I'll take care of this section in one of the other PRs. If you would, go ahead and pull this particular bit back out :-)

I've taken it out in the updated commit

joyeecheung · 2017-02-10T19:23:43Z

doc/api/errors.md

+### ERR_NO_DOMAIN
+
+An error using the `'ERR_NO_DOMAIN'` code is thrown specifically when an attempt 
+is made to parse an url without a domain name


nit: without a valid domain name

joyeecheung · 2017-02-15T16:13:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6425/

seppevs · 2017-02-22T09:24:21Z

The github jenkins bot (see comment with "some checks were not successful") reports that test/arm failed. But when I click through, I see that the build was successful? https://ci.nodejs.org/job/node-test-commit-arm/7774/console

seppevs · 2017-05-01T20:24:09Z

Rebased

joyeecheung · 2017-05-02T15:14:45Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/7801/

jasnell · 2017-05-02T21:56:17Z

@nodejs/ctc ... this is ready to go, but as a semver-major needs another CTC member to sign off.

Trott

LGTM if CI is green

Trott · 2017-05-02T23:29:32Z

doc/api/errors.md

+### ERR_NO_DOMAIN
+
+An error using the `'ERR_NO_DOMAIN'` code is thrown specifically when an attempt 
+is made to parse an url without a valid domain name


Nit: url -> URL

Nit: Period at the end of the sentence.

Trott · 2017-05-02T23:30:09Z

lib/internal/errors.js

@@ -111,6 +111,7 @@ E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected');
 E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe');
 E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks');
 E('ERR_MISSING_ARGS', missingArgs);
+E('ERR_NO_DOMAIN', 'Unable to determine the domain name');


Nit: Maybe drop the word the?

Although I see it was a copy/paste from the existing error message, so certainly feel free to keep it that way if you'd prefer...

Trott

LGTM if CI is green. Left a few minor comments on prose that should be addressed IMO.

Trott

LGTM if CI is green. Would like the nits about URL and a period (full stop) addressed, but those can be done when landing probably.

seppevs · 2017-05-03T06:52:10Z

Changes done

Trott · 2017-05-04T03:04:46Z

doc/api/errors.md

@@ -558,7 +558,7 @@ found [here][online].
  the connected party did not properly respond after a period of time. Usually
  encountered by [`http`][] or [`net`][] -- often a sign that a `socket.end()`
  was not properly called.
-
+  


Micro-nit: stray space can be removed?

joyeecheung · 2017-05-04T11:18:19Z

Looks like this is breaking test-benchmark-crypto.js on win2016...

Error: Cannot find module 'c:\workspace\node-test-binary-windows\RUN_SUBSET\0\VS_VERSION\vs2015\label\win2016\benchmark\crypto\hash-stream-creation.js'

Not sure how this happens..just in case it's something wrong with the build step, have another CI: https://ci.nodejs.org/job/node-test-pull-request/7847/

jasnell · 2017-05-04T12:08:37Z

doc/api/errors.md

+### ERR_NO_DOMAIN
+
+An error using the `'ERR_NO_DOMAIN'` code is thrown specifically when an attempt 
+is made to parse an URL without a valid domain name.


Perhaps ERR_HTTP_NO_DOMAIN to make the error more specific to the context.

fhinkel · 2017-05-23T18:51:55Z

@seppevs Do you want to address @jasnell 's comment and rebase? Thanks!

seppevs · 2017-05-24T07:28:40Z

@fhinkel done

jasnell · 2017-05-24T10:34:07Z

Are there no test cases that check for this error? If not, one could definitely be added.

fhinkel · 2017-06-07T16:53:48Z

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.

refack · 2017-07-19T22:14:48Z

I'll follow up

BridgeAR · 2017-09-20T00:41:07Z

I am closing this due to the long inactivity. @refack please do not reopen in case there is no further progress.

@seppevs if you would like to follow up on this, please leave a comment. Thanks a lot for your contribution anyways.

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. https Issues or PRs related to the https subsystem. labels Feb 10, 2017

jasnell requested changes Feb 10, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 10, 2017

jasnell mentioned this pull request Feb 10, 2017

Tracking Issue: Migrate errors to internal/errors.js #11273

Closed

80 tasks

seppevs force-pushed the https_use_internal_errors branch from 5978a84 to 053ac8d Compare February 10, 2017 19:10

joyeecheung reviewed Feb 10, 2017

View reviewed changes

seppevs force-pushed the https_use_internal_errors branch 3 times, most recently from 2e5bcaa to 1395a3f Compare February 10, 2017 21:10

joyeecheung approved these changes Feb 11, 2017

View reviewed changes

jasnell approved these changes Feb 11, 2017

View reviewed changes

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

seppevs force-pushed the https_use_internal_errors branch from 1395a3f to 8b7f7ec Compare May 1, 2017 20:24

Trott approved these changes May 2, 2017

View reviewed changes

Trott reviewed May 2, 2017

View reviewed changes

Trott requested changes May 2, 2017

View reviewed changes

Trott approved these changes May 2, 2017

View reviewed changes

seppevs force-pushed the https_use_internal_errors branch from 8b7f7ec to d6c3bc0 Compare May 3, 2017 06:51

Trott reviewed May 4, 2017

View reviewed changes

joyeecheung approved these changes May 4, 2017

View reviewed changes

jasnell requested changes May 4, 2017

View reviewed changes

https: use internal/errors.js

1615093

seppevs force-pushed the https_use_internal_errors branch from d6c3bc0 to 1615093 Compare May 24, 2017 07:28

fhinkel closed this Jun 7, 2017

refack reopened this Jul 19, 2017

refack self-assigned this Jul 19, 2017

BridgeAR closed this Sep 20, 2017

refack removed their assignment Oct 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

https: use internal/errors.js #11293

https: use internal/errors.js #11293

seppevs commented Feb 10, 2017 •

edited

Loading

jasnell left a comment

jasnell Feb 10, 2017

jasnell commented Feb 10, 2017

seppevs commented Feb 10, 2017

joyeecheung Feb 10, 2017 •

edited

Loading

jasnell Feb 10, 2017

seppevs Feb 10, 2017

joyeecheung Feb 10, 2017

seppevs Feb 10, 2017

joyeecheung commented Feb 15, 2017

seppevs commented Feb 22, 2017 •

edited

Loading

seppevs commented May 1, 2017

joyeecheung commented May 2, 2017

jasnell commented May 2, 2017

Trott left a comment

Trott May 2, 2017

Trott May 2, 2017

Trott May 2, 2017

Trott May 2, 2017

Trott left a comment

Trott left a comment

seppevs commented May 3, 2017

Trott May 4, 2017

joyeecheung commented May 4, 2017

jasnell May 4, 2017

fhinkel commented May 23, 2017

seppevs commented May 24, 2017

jasnell commented May 24, 2017

fhinkel commented Jun 7, 2017

refack commented Jul 19, 2017

BridgeAR commented Sep 20, 2017

https: use internal/errors.js #11293

https: use internal/errors.js #11293

Conversation

seppevs commented Feb 10, 2017 • edited Loading

Checklist

Affected core subsystem(s)

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Feb 10, 2017

seppevs commented Feb 10, 2017

joyeecheung Feb 10, 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

joyeecheung commented Feb 15, 2017

seppevs commented Feb 22, 2017 • edited Loading

seppevs commented May 1, 2017

joyeecheung commented May 2, 2017

jasnell commented May 2, 2017

Trott left a 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

Trott left a comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

seppevs commented May 3, 2017

Choose a reason for hiding this comment

joyeecheung commented May 4, 2017

Choose a reason for hiding this comment

fhinkel commented May 23, 2017

seppevs commented May 24, 2017

jasnell commented May 24, 2017

fhinkel commented Jun 7, 2017

refack commented Jul 19, 2017

BridgeAR commented Sep 20, 2017

seppevs commented Feb 10, 2017 •

edited

Loading

joyeecheung Feb 10, 2017 •

edited

Loading

seppevs commented Feb 22, 2017 •

edited

Loading