errors: improve invalid arg type #13834

BridgeAR · 2017-06-21T01:07:03Z

I reworked the ERR_INVALID_ARG_TYPE error type in a way that provides more information to the user than before.
All of these errors now return what was actually provided and failed.
The information about what was provided and what is expected is more specific (as in not only the type is checked but also the constructor).
I also fixed a few typos and wrong usages like using typeof arg as actual value instead of arg and one entry that actually checks a property, not an argument.

There is one point that might be thought about: mixing multiple expected values can only print either instance of or type of. So ['Array', 'string'] will result in instance of Array or string and not instance of Array or type of string. I could test each entry but I feel that is somewhat unnecessary?

Checklist

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

Affected core subsystem(s)

errors

BridgeAR · 2017-06-21T01:18:23Z

Ref #13730
Ref #13733 (one error is actually not touched here and is fixed in this PR instead).

mscdex · 2017-06-21T01:25:27Z

lib/internal/errors.js

+ if (actual == null) {
+ received = String(actual);
+ } else if (typeof actual === 'object') {
+ if (actual.constructor !== undefined) {


You may want to use single equals to also check for null.

Hm, as far as I can tell someone would have to explicitly set the constructor to null... that would be really weird but I'll change it accordingly.

mscdex · 2017-06-21T01:26:15Z

Marking as semver-major due to changes in error messages.

BridgeAR · 2017-06-21T01:30:17Z

@mscdex this should not be semver major as the main intention of the internal errors was to change error messages when ever.

mscdex · 2017-06-21T01:33:16Z

@BridgeAR Right, that's the end goal, but I think until the new error mechanism is widely adopted (e.g. users start checking the special error properties instead of .message) we're still marking error message changes as semver-major. /cc @jasnell to be sure.

mhdawson · 2017-06-23T16:15:14Z

Like the improvement. Agree we should still be making these changes semver-major until we get overall agreement that we can starting treating error messages changes differently.

Also needs a rebase.

refack · 2017-06-23T21:49:23Z

Like the improvement. Agree we should still be making these changes semver-major until we get overall agreement that we can starting treating error messages changes differently.

There are mixed messages being conveyed #13730 (comment)... IMHO the CTC should discuss and decide.
ping @jasnell

BridgeAR · 2017-06-26T13:08:11Z

Rebased

refack · 2017-06-26T13:30:42Z

IMHO in the tests when you assert the message it should be a RegEx anchored with start-of-line and end in "Received": /^The "last argument" argument must be of type function\. Received/ because whatever comes after "Received" is dependent of the actual input, while the rest is static for each case.

refack · 2017-06-26T13:52:08Z

Another idea: export a function from errors: invalidArg(opts) that you use as error.invalidArgs({expected: "gaga", bar}); then you can infer the type of the second arg's name and value:

function invalidArg(opts) {
  assert.strictEqual(Object.keys(opts).length, 2);
  assert.strictEqual('expected' in opts, true);
  const exp = opts.expected;
  delete opts.expected;
  const actKey = Object.keys(opts)[0];
  const actVal = opts[actKey];
  throw new TypeError('ERR_INVALID_ARG_TYPE', actKey, exp, actVal
// if we add a stackLimit arg
// ,invalidArg);
}

refack

LGTM

refack · 2017-06-26T13:23:51Z

test/parallel/test-process-cpuUsage.js

@@ -34,13 +34,13 @@ for (let i = 0; i < 10; i++) {
 const invalidUserArgument = common.expectsError({
 code: 'ERR_INVALID_ARG_TYPE',
 type: TypeError,
- message: 'The "preValue.user" property must be of type Number'
+ message: /The "prevValue\.user" property must be of type number/


Could you anchor these /^...

refack · 2017-06-26T13:33:08Z

lib/util.js

+ throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
+ 'superCtor.prototype',
+ 'function',
+ superCtor.prototype);


IMHO for readability use undefined

I personally prefer it this way to have a consistent way of doing this but I don't have strong feelings about it.

👍 Generally I yield to the OP. So it's your call.

refack · 2017-06-26T13:36:09Z

lib/util.js

@@ -205,7 +205,8 @@ Object.defineProperty(inspect, 'defaultOptions', {
 },
 set: function(options) {
 if (options === null || typeof options !== 'object') {
- throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object');
+ throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'object',


Personal style nit: Is these cases chop down the args like in L1082

BridgeAR · 2017-06-26T20:56:17Z

@refack

you can infer the type of the second arg's name and value

I feel like this is a bit to much magic and the call itself won't be much shorter. So I'd say let's stick to how it is currently.

I also added the Received part in the RegExp if the line length allowed it. I hope that's fine, I personally don't see much benefit in adding the Received as it might theoretically change and it is already checked that all arguments are properly passed to the internal error.

refack · 2017-06-26T21:21:37Z

you can infer the type of the second arg's name and value

I feel like this is a bit to much magic and the call itself won't be much shorter. So I'd say let's stick to how it is currently.

👍

I also added the Received part in the RegExp if the line length allowed it. I hope that's fine, I personally don't see much benefit in adding the Received as it might theoretically change and it is already checked that all arguments are properly passed to the internal error.

Just so it doesn't change "accidentally". Changes in test files get more review attention, so IMHO it's best to assert as much as possible, then later change carefully.

refack · 2017-06-26T21:34:26Z

Sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/6883/

BridgeAR · 2017-06-29T11:27:52Z

Rebased

mhdawson

LGTM with a question. In a number of tests the check stops at 'Received' any reason we can't validate the additional information after 'Received' as well ?

BridgeAR · 2017-06-29T21:51:20Z

It is in most cases possible to use the exact value. In the end I don't think that it's that important to test for the message strictly as it can change more often but I'm fine with changing it.

nodejs-github-bot added dont-land-on-v4.x errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jun 21, 2017

BridgeAR changed the title ~~Improve invalid arg type~~ errors: improve invalid arg type Jun 21, 2017

mscdex reviewed Jun 21, 2017

View reviewed changes

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

refack mentioned this pull request Jun 25, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-06-28 nodejs/CTC#148

Closed

BridgeAR force-pushed the improve-invalid-arg-type branch from c180fec to 63c5700 Compare June 26, 2017 12:11

refack reviewed Jun 26, 2017

View reviewed changes

refack mentioned this pull request Jun 26, 2017

meta: semver impact of Error messages #13937

Closed

BridgeAR force-pushed the improve-invalid-arg-type branch from 63c5700 to 0d4204c Compare June 26, 2017 20:51

refack approved these changes Jun 26, 2017

View reviewed changes

BridgeAR force-pushed the improve-invalid-arg-type branch from 0d4204c to dc4e540 Compare June 29, 2017 11:27

mhdawson approved these changes Jun 29, 2017

View reviewed changes

snyk-bot mentioned this pull request Feb 12, 2022

[Snyk] Fix for 9 vulnerabilities Iceymann18777/node#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: improve invalid arg type #13834

errors: improve invalid arg type #13834

BridgeAR commented Jun 21, 2017

BridgeAR commented Jun 21, 2017

mscdex Jun 21, 2017

BridgeAR Jun 21, 2017

mscdex commented Jun 21, 2017

BridgeAR commented Jun 21, 2017

mscdex commented Jun 21, 2017 •

edited

Loading

mhdawson commented Jun 23, 2017

refack commented Jun 23, 2017

BridgeAR commented Jun 26, 2017

refack commented Jun 26, 2017

refack commented Jun 26, 2017

refack left a comment

refack Jun 26, 2017

BridgeAR Jun 26, 2017

refack Jun 26, 2017

BridgeAR Jun 26, 2017

refack Jun 26, 2017

refack Jun 26, 2017

BridgeAR Jun 26, 2017

BridgeAR commented Jun 26, 2017

refack commented Jun 26, 2017

refack commented Jun 26, 2017

BridgeAR commented Jun 29, 2017

mhdawson left a comment

BridgeAR commented Jun 29, 2017

errors: improve invalid arg type #13834

errors: improve invalid arg type #13834

Conversation

BridgeAR commented Jun 21, 2017

Checklist

Affected core subsystem(s)

BridgeAR commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jun 21, 2017

BridgeAR commented Jun 21, 2017

mscdex commented Jun 21, 2017 • edited Loading

mhdawson commented Jun 23, 2017

refack commented Jun 23, 2017

BridgeAR commented Jun 26, 2017

refack commented Jun 26, 2017

refack commented Jun 26, 2017

refack 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jun 26, 2017

refack commented Jun 26, 2017

refack commented Jun 26, 2017

BridgeAR commented Jun 29, 2017

mhdawson left a comment

Choose a reason for hiding this comment

BridgeAR commented Jun 29, 2017

mscdex commented Jun 21, 2017 •

edited

Loading