-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
url: port WHATWG URL API to internal/errors #12574
Conversation
@@ -561,6 +561,13 @@ found [here][online]. | |||
<a id="nodejs-error-codes"></a> | |||
## Node.js Error Codes | |||
|
|||
<a id="ERR_ARG_NOT_ITERABLE"></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be just ERR_INVALID_ARG_TYPE
with Iterable
as the expected type..no strong feelings about it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this I think the more specific error is warranted.
@@ -840,15 +841,17 @@ class URLSearchParams { | |||
for (const pair of init) { | |||
if (typeof pair !== 'object' || | |||
typeof pair[Symbol.iterator] !== 'function') { | |||
throw new TypeError('Each query pair must be iterable'); | |||
throw new errors.TypeError('ERR_INVALID_TUPLE', 'Each query pair', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the name/value
thing is already in error messages...but since we are changing error messages anyway, I think [name, value]
would be easier to understand at the first glance.
doc/api/errors.md
Outdated
### ERR_INVALID_THIS | ||
|
||
The `'ERR_INVALID_THIS'` error code is used generically to identify that a | ||
Node.js API function is called with an incompatible `this` value. Its use is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "limited to WHATWG URL" part can be left out IMO, the source of errors would be visible in the stack trace anyway... This way people don't need to change this section when they add this type of error to other APIs
doc/api/errors.md
Outdated
|
||
An error with code `'ERR_INVALID_TUPLE'` is thrown when an element in the | ||
`iterable` provided to the [WHATWG][WHATWG URL API] [`URLSearchParams` | ||
constructor][`new URLSearchParams(iterable)`] does not represent a name/value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[name, value] tuple
would be easier to grasp..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"name/value tuple" is the WHATWG nomenclature, but I'm perfectly fine with "[name, value]
tuple".
An error using the `'ERR_INVALID_URL'` code is thrown when an invalid URL is | ||
passed to the [WHATWG][WHATWG URL API] [`URL` constructor][`new URL(input)`] to | ||
be parsed. The thrown error object typically has an additional property | ||
`'input'` that contains the URL that failed to parse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that at the moment input
might not be exactly the same as the original input since it needs to go through trimming and String::NO_NULL_TERMINATION | String::REPLACE_INVALID_UTF8
first...we could've stored the original input, but that's for another PR to consider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the REPLACE_INVALID_UTF8
is pretty edge-case, but trimming can indeed be something the user does not expect. Agreed on addressing this in a separate PR though.
in the [WHATWG URL API][] for strict compliance with the specification (which | ||
in some cases may accept `func(undefined)` but not `func()`). In most native | ||
Node.js APIs, `func(undefined)` and `func()` are treated identically, and the | ||
[`ERR_INVALID_ARG_TYPE`][] error code may be used instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, why not just use 'ERR_INVALID_ARG_TYPE'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'ERR_INVALID_ARG_TYPE'
has the following templated message:
The "FOO" argument must be of type BAR.
In this case, it is not the question of what type the argument is, but whether it is defined at all.
lib/internal/errors.js
Outdated
E('ERR_ASSERTION', (msg) => msg); | ||
E('ERR_INVALID_ARG_TYPE', invalidArgType); | ||
E('ERR_INVALID_CALLBACK', 'callback must be a function'); | ||
E('ERR_INVALID_FILE_URL_HOST', 'File URL host must %s'); | ||
E('ERR_INVALID_FILE_URL_PATH', 'File URL path must %s'); | ||
E('ERR_INVALID_THIS', 'Value of this must be of type %s'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe surround this
with quotes or backticks? The original error message uses backticks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite rare to have an error message with backticks in it (most error messages use double quotes). Here are a few alternatives:
- Value of "this" must be of type %s
- Method called on incompatible receiver of type %s (V8-inspired)
- Method called on incompatible %s (SpiderMonkey-inspired)
Choice 1 SGTM.
lib/internal/errors.js
Outdated
E('ERR_INVALID_THIS', 'Value of this must be of type %s'); | ||
E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple'); | ||
E('ERR_INVALID_URL', | ||
(input) => `Invalid URL${input !== undefined ? `: ${input}` : ''}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think undefined
should be in the message here..otherwise if the user makes a mistake and passes an undefined, there is no way to know if it is an undefined or just an empty string.
lib/internal/url.js
Outdated
return new TypeError( | ||
'Path must not include encoded \\ or / characters'); | ||
return new errors.TypeError('ERR_INVALID_FILE_URL_PATH', | ||
'not include encoded \\ or / characters'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird to put the verbs in separate places...maybe the must
part can be left out in the error message format and written here?
return typeof err === 'object' && 'name' in err && err.name === code.name; | ||
return typeof err === 'object' && | ||
'name' in err && | ||
err.name.startsWith(code.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe err instanceof code.constructor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about this one, I'm generally not a huge fan of instanceof
checks due to unreliability across different V8 contexts, and it is quite possible that WPT's testharness.js uses the error name string check instead of instanceof
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see @joyeecheung's comments addressed.
Also slightly revise the grammar of two-expected value scenario.
Addressed most of @joyeecheung's comments, except those that are still visible above. |
Landed in d457a98. |
Refs: #11273
Refs: #11299
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
url, errors