http_client, errors: migrate to use internal/errors by starkwang · Pull Request #14423 · nodejs/node · GitHub

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, http_client

@starkwang thank you so much for helping with this!

We recently updated the recommended way to test internal Errors — https://github.com/nodejs/node/pull/14207/files
tl;dr need to test the message only if it's variable, and once is enough (in test/parallel/test-internal-errors.js).
IMHO the PR is good as is, but it would be great if you followed up and made it even better.

/cc @jasnell @fhinkel @Trott @mhdawson

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

FYI you could just use %s place holders, but IMHO this way is more readable.

But: consistency 😉

There is none anyway...
I'll open a PR to change them all.

Whoever lands this: I think the subsystem should be http_client with an underscore.

@refack I've added some tests for the new errors in test/parallel/test-internal-errors.js😀

semver-major so pinging @nodejs/ctc for some reviews

💯

LGTM

@starkwang Sorry to be a bummer, but can you rebase this against master? There are some merge conflicts here…

It seems that I missing some titles for errors.md in #14212 . So I added the missing titles in this PR

@addaleax I've just rebased the branch : )

Fresh CI: https://ci.nodejs.org/job/node-test-commit/11395/

From using-internal-errors.md:

If the error message is not a constant string then tests to validate the formatting of the message based on the parameters used when creating the error should be added to test/parallel/test-internal-errors.js. These tests should validate all of the different ways parameters can be used to generate the final message string.

And this section:

In addition, there should also be tests which validate the use of the error based on where it is used in the codebase. For these tests, except in special cases, they should only validate that the expected code is received and NOT validate the message.

I don't exactly care about this, just remember that this PR does not strictly comply with these guidelines.

Thanks for your efforts @starkwang! 😃

Unless I am missing something, it does not appear to be necessary to use a regular expression here, a string literal should work just fine. By the way, if you want to match an exact string using regular expressions, use start (^) and end markers ($). Otherwise, the match will succeed even if there is content before or after the expression:

const expr = /The "method" argument must be of type string/;
const str = 'The "method" argument must be of type string. Also, this sentence should not be here';
expr.test(str) === true

Same here, why is this a regular expression?

Same here.

Grammar: have been sent. Also, http probably refers to the protocol instead of the module name, so I would prefer HTTP over http.

Used when new HTTP headers are added to a response, after the headers part has already been sent.

http probably refers to the protocol instead of the module name, so I would prefer HTTP over http.

Grammar: that be passed. I would just say "... failed to parse the host or hostname option".

Used when `hostname` can not be parsed from a provided URL.

Grammar: when receive. Should probably be when receiving.

Used when a string that contains unescaped characters was received.

I don't have a strong opinion on this, I would prefer a more meaningful name for this error code. From the error code, the actual reason of the error is unclear, and the message implies that having the headers sent already is the only reason sending headers can fail, ever, and I am not too sure about that. Maybe something like ERR_HEADERS_SENT? Doesn't sound perfect either... cc @refack

ERR_HTTP_HEADERS_SENT

IMHO the documentation improvements should be non blockers, since simply migrating the Errors is of higher priority. But it's definatly nice to have while we wait for another @nodejs/ctc approval.

This PR has http, internal errors, and is semver major. Seems like @jasnell should be all over it. Ping! :-D

Ref: nodejs#11273

Pushed commit to address comments

💯

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

Landed in bdfbce9.

PR-URL: #14423 Refs: #11273 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This error code originally landed in a semver-major commit and is used by the ESM implementation. This backport includes the error message and the documentation for the error. I did attempt to write a test for this, but it did not seem possible to catch an exception during import, I was also unable to execute `node --experimental-modules` properly inside of a child_process. I'll dig more into getting a test together, but we should backport this fix in the mean time. Refs: nodejs#14423 Fixes: nodejs#15374

This error code originally landed in a semver-major commit and is used by the ESM implementation. This backport includes the error message and the documentation for the error. I did attempt to write a test for this, but it did not seem possible to catch an exception during import, I was also unable to execute `node --experimental-modules` properly inside of a child_process. I'll dig more into getting a test together, but we should backport this fix in the mean time. Refs: #14423 Fixes: #15374

This error code originally landed in a semver-major commit and is used by the ESM implementation. This backport includes the error message and the documentation for the error. I did attempt to write a test for this, but it did not seem possible to catch an exception during import, I was also unable to execute `node --experimental-modules` properly inside of a child_process. I'll dig more into getting a test together, but we should backport this fix in the mean time. Refs: #14423 Fixes: #15374 PR-URL: #15388 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

refack approved these changes Jul 22, 2017

View reviewed changes

jasnell mentioned this pull request Jul 22, 2017

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

Closed

80 tasks

refack reviewed Jul 22, 2017

View reviewed changes

starkwang force-pushed the http-client-internal-errors branch from 6031cca to 4ad89b7 Compare July 22, 2017 16:29

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 22, 2017

refack approved these changes Jul 22, 2017

View reviewed changes

mcollina approved these changes Jul 23, 2017

View reviewed changes

starkwang force-pushed the http-client-internal-errors branch 2 times, most recently from c54858b to 4c62748 Compare July 27, 2017 02:20

starkwang commented Jul 27, 2017

View reviewed changes

tniessen reviewed Jul 27, 2017

View reviewed changes

starkwang force-pushed the http-client-internal-errors branch 3 times, most recently from 58fd145 to d4c91a7 Compare July 28, 2017 02:42

starkwang changed the title http client, errors: migrate to use internal/errors http_client, errors: migrate to use internal/errors Jul 28, 2017

tniessen reviewed Jul 28, 2017

View reviewed changes

http_client, errors: migrate to use internal/errors

db8fb2a

Ref: nodejs#11273

starkwang force-pushed the http-client-internal-errors branch from d4c91a7 to db8fb2a Compare July 30, 2017 13:41

refack approved these changes Jul 30, 2017

View reviewed changes

tniessen approved these changes Jul 30, 2017

View reviewed changes

jasnell approved these changes Jul 31, 2017

View reviewed changes

tniessen closed this Aug 1, 2017

MylesBorins mentioned this pull request Sep 13, 2017

errors: backport ERR_INVALID_PROTOCOL to v8.x #15388

Closed

Trott mentioned this pull request Oct 4, 2017

test: fix assert.throws in test-http-invalid-urls #15678

Closed

2 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

http_client, errors: migrate to use internal/errors by starkwang · Pull Request #14423 · nodejs/node · GitHub

Uh oh!

Uh oh!

http_client, errors: migrate to use internal/errors - #14423

http_client, errors: migrate to use internal/errors#14423starkwang wants to merge 1 commit intonodejs:masternodejs/node:masterfrom starkwang:http-client-internal-errorsstarkwang/node:http-client-internal-errorsCopy head branch name to clipboard

Conversation

starkwang commented Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Jul 22, 2017

Uh oh!

refack commented Jul 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen commented Jul 22, 2017

Uh oh!

starkwang commented Jul 22, 2017

Uh oh!

Trott commented Jul 22, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starkwang commented Jul 27, 2017

Uh oh!

addaleax commented Jul 27, 2017

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Jul 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 29, 2017

Uh oh!

Trott commented Jul 29, 2017

Uh oh!

starkwang commented Jul 30, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

http_client, errors: migrate to use internal/errors#14423
starkwang wants to merge 1 commit into
nodejs:masterfrom
starkwang:http-client-internal-errors

starkwang commented Jul 22, 2017 •
edited
Loading

refack Jul 29, 2017 •
edited
Loading