Skip to content
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.

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

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

Closed

Conversation

@starkwang
Copy link
Contributor

@starkwang starkwang commented Jul 22, 2017

Ref: #11273

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

@refack
refack approved these changes Jul 22, 2017
@refack
Copy link
Member

@refack refack commented Jul 22, 2017

@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

@jasnell jasnell mentioned this pull request Jul 22, 2017
79 of 80 tasks complete
E('ERR_INVALID_FD', '"fd" must be a positive integer: %s');
E('ERR_INVALID_FILE_URL_HOST',
'File URL host must be "localhost" or empty on %s');
E('ERR_INVALID_FILE_URL_PATH', 'File URL path %s');
E('ERR_INVALID_HANDLE_TYPE', 'This handle type cannot be sent');
E('ERR_INVALID_HTTP_TOKEN', (name) => `${name} must be a valid HTTP token`);

This comment has been minimized.

@refack

refack Jul 22, 2017
Member

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

This comment has been minimized.

@tniessen

tniessen Jul 22, 2017
Member

But: consistency 😉

This comment has been minimized.

@refack

refack Jul 22, 2017
Member

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

@tniessen
Copy link
Member

@tniessen tniessen commented Jul 22, 2017

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

@starkwang starkwang force-pushed the starkwang:http-client-internal-errors branch Jul 22, 2017
@starkwang
Copy link
Contributor Author

@starkwang starkwang commented Jul 22, 2017

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

@Trott Trott added the semver-major label Jul 22, 2017
@Trott
Copy link
Member

@Trott Trott commented Jul 22, 2017

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

@refack
refack approved these changes Jul 22, 2017
Copy link
Member

@refack refack left a comment

💯

Copy link
Member

@mcollina mcollina left a comment

LGTM

@addaleax
Copy link
Member

@addaleax addaleax commented Jul 26, 2017

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

@starkwang starkwang force-pushed the starkwang:http-client-internal-errors branch 2 times, most recently Jul 27, 2017
@@ -597,6 +597,7 @@ Used when `Console` is instantiated without `stdout` stream or when `stdout` or
Used when the native call from `process.cpuUsage` cannot be processed properly.

<a id="ERR_DNS_SET_SERVERS_FAILED"></a>
### ERR_DNS_SET_SERVERS_FAILED

This comment has been minimized.

@starkwang

starkwang Jul 27, 2017
Author Contributor

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

doc/api/errors.md Outdated
<a id="ERR_INVALID_IP_ADDRESS"></a>
### ERR_INVALID_IP_ADDRESS

This comment has been minimized.

@starkwang

starkwang Jul 27, 2017
Author Contributor

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

@starkwang
Copy link
Contributor Author

@starkwang starkwang commented Jul 27, 2017

@addaleax I've just rebased the branch : )

Copy link
Member

@tniessen tniessen left a comment

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! 😃

test/parallel/test-http-client-check-http-token.js Outdated
}, common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: /The "method" argument must be of type string/

This comment has been minimized.

@tniessen

tniessen Jul 27, 2017
Member

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
test/parallel/test-http-hostname-typechecking.js Outdated
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: new RegExp('The "options\\.hostname" property must be one of ' +
'type string, undefined, or null')

This comment has been minimized.

@tniessen

tniessen Jul 27, 2017
Member

Same here, why is this a regular expression?

test/parallel/test-http-hostname-typechecking.js Outdated
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: new RegExp('The "options\\.host" property must be one of ' +
'type string, undefined, or null')

This comment has been minimized.

@tniessen

tniessen Jul 27, 2017
Member

Same here.

@starkwang starkwang force-pushed the starkwang:http-client-internal-errors branch 3 times, most recently Jul 28, 2017
@starkwang starkwang changed the title http client, errors: migrate to use internal/errors http_client, errors: migrate to use internal/errors Jul 28, 2017
doc/api/errors.md Outdated
<a id="ERR_RENDER_HEADERS_FAILED"></a>
### ERR_RENDER_HEADERS_FAILED

Used when http headers has been sent before rendering the headers.

This comment has been minimized.

@tniessen

tniessen Jul 28, 2017
Member

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

This comment has been minimized.

@refack

refack Jul 29, 2017
Member

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

doc/api/errors.md Outdated
<a id="ERR_INVALID_HTTP_TOKEN"></a>
### ERR_INVALID_HTTP_TOKEN

Used when `options.method` received an invalid http token.

This comment has been minimized.

@tniessen

tniessen Jul 28, 2017
Member

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

doc/api/errors.md Outdated
<a id="ERR_INVALID_DOMAIN_NAME"></a>
### ERR_INVALID_DOMAIN_NAME

Used when `url.parse()` failed to parse the `host` or `hostname` that be passed to

This comment has been minimized.

@tniessen

tniessen Jul 28, 2017
Member

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

This comment has been minimized.

@refack

refack Jul 29, 2017
Member

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

doc/api/errors.md Outdated
<a id="ERR_UNESCAPED_CHARACTERS"></a>
### ERR_UNESCAPED_CHARACTERS

Used when receive a string that contains unescaped characters.

This comment has been minimized.

@tniessen

tniessen Jul 28, 2017
Member

Grammar: when receive. Should probably be when receiving.

This comment has been minimized.

@refack

refack Jul 29, 2017
Member

Used when a string that contains unescaped characters was received.

lib/_http_client.js Outdated
@@ -303,7 +302,7 @@ ClientRequest.prototype._finish = function _finish() {

ClientRequest.prototype._implicitHeader = function _implicitHeader() {
if (this._header) {
throw new Error('Can\'t render headers after they are sent to the client');
throw new errors.Error('ERR_RENDER_HEADERS_FAILED');

This comment has been minimized.

@tniessen

tniessen Jul 28, 2017
Member

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

This comment has been minimized.

@refack

refack Jul 29, 2017
Member

ERR_HTTP_HEADERS_SENT

@refack
Copy link
Member

@refack refack commented Jul 29, 2017

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.

@Trott
Copy link
Member

@Trott Trott commented Jul 29, 2017

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

@starkwang starkwang force-pushed the starkwang:http-client-internal-errors branch to db8fb2a Jul 30, 2017
@starkwang
Copy link
Contributor Author

@starkwang starkwang commented Jul 30, 2017

Pushed commit to address comments

@refack
refack approved these changes Jul 30, 2017
Copy link
Member

@refack refack left a comment

💯

@tniessen
Copy link
Member

@tniessen tniessen commented Aug 1, 2017

Landed in bdfbce9.

@tniessen tniessen closed this Aug 1, 2017
tniessen added a commit that referenced this pull request Aug 1, 2017
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>
@refack refack added this to Done in Error Codes Aug 20, 2017
MylesBorins added a commit to MylesBorins/node that referenced this pull request Sep 13, 2017
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
MylesBorins added a commit that referenced this pull request Sep 14, 2017
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
MylesBorins added a commit that referenced this pull request Sep 14, 2017
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>
@Trott Trott mentioned this pull request Oct 4, 2017
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Error Codes
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.