errors: improve ERR_INVALID_ARG_TYPE #29675

BridgeAR · 2019-09-23T17:48:27Z

Commit message 1: buffer: improve .from() error details

This makes sure the original input is passed to the error in case
no matching inputs are found. Instead of passing along all values,
only valid or possibliy valid values are passed through. That way
invalid values end up in the error case with the original input.

Commit message 2: errors: improve ERR_INVALID_ARG_TYPE

ERR_INVALID_ARG_TYPE is the most common error used throughout the
code base. This improves the error message by providing more details
to the user and by indicating more precisely which values are allowed
ones and which ones are not.

It adds the actual input to the error message in case it's a primitive.
If it's a class instance, it'll print the class name instead of
"object" and "falsy" or similar entries are not named "type" anymore.

The first commit is necessary to get the tests to pass. Otherwise the
error would not log the actual input but the unboxed value.

I am not completely certain if we should handle Object special instead of just handling it as any other regular type. Any options on that?

// cc @Trott

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

nodejs-github-bot · 2019-09-23T20:25:23Z

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

Trott · 2019-09-24T04:36:57Z

lib/_http_server.js

@@ -313,7 +313,7 @@ function Server(options, requestListener) {
  } else if (options == null || typeof options === 'object') {
    options = { ...options };
  } else {
-    throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
+    throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);


What's the justification for changing object to Object? Here are the things that come to mind as arguments against it:

typeof {} === 'object' and not 'Object'

Sure, you can do new Object() but you can also do new String() or new Number() and we don't capitalize those.

"Everything is lowercased" is an easy rule to remember. "Some things get an initial capital letter and other things don't" is more prone to human error.

No added value to the end user of saying Object instead of object.

Not doing that change will greatly reduce the diff size, making this easier to review as well as backport. It also reduces the risk of conflicts with other PRs.

The justification was that there are already multiple entries written as Object and the documentation uses Object instead of object. I personally actually favor object a tiny bit above Object. I just roughly remembered that there where comments about this in a PR quite a while ago (I do not remember which one though), so I thought I'll go for that.

This actually also applies to the type function. It is mostly written as Function.

@vsemozhetbyt I think you where involved in this discussion. Do you by any chance remember any details?

Sorry, I cannot remember. I can only say that we use lower case for primitives and upper case for other types in doc parts that are based on tools/doc/type-parser.js, otherwise I cannot decide what is better for code and error messages.

@Trott I am fine to convert all Function and Object entries to lower cased versions, if you think that's a good way to handle it. I personally favor that.

@Trott I am fine to convert all Function and Object entries to lower cased versions, if you think that's a good way to handle it. I personally favor that.

I'm fine either way.

when talking about the type I expect it to correspond to typeof input === 'object'. When Object is used I would expect the direct prototype to be `Object

I might be misunderstanding you, but those seem like arguments for keeping lowercase object here, because it is about typeof and not the prototype constructor name?

It's correct that we use typeof input === 'object' in (probably) all of these cases. But when we do, we normally mean the object should have Object as prototype (or a null prototype as in Object.create(null)).

The example above is more intuitive for me if it's written as:
The "error" argument must be of type function or an instance of Object, Error, or RegExp. That way I would immediately expect the input to be allowed to look like:

let input = {} // Valid input = /a/ // Valid input = new TypeError() // Valid input = new Map() // Invalid

@addaleax what do you think would be best here? The PR should overall reduce ambiguity and seems clearer than before.

But when we do, we normally mean the object should have Object as prototype (or a null prototype as in Object.create(null)).

I don’t think that’s true, and the way that JS objects work, the Map instance in your example is valid because we can attach regular properties to it, too.

So I’d personally stick with the current formatting of lowercase object unless we actually perform some kind of instanceof-style check.

You are correct that it would be possible to use any object if we do the typecheck only. What we normally do is a check for a specific class, than another one and in the end we accept any object as long as it seems to fit.

However, we do mostly not accept null and that is also of type object.

I decided to go for a middle ground and hope that is good for everyone: if no class is detected, we'll always use type in combination with object. If we detect a class and object as accepted values, we'll declare it as instance of Object. This condition is pretty rare and I could only find few errors where this would apply too. That way we'll keep the distinction between different object types and use type of object in almost all other cases.

Is that fine for you?

Trott · 2019-10-01T04:36:33Z

@nodejs/collaborators This could use some reviews.

Trott · 2019-12-19T15:43:06Z

test/es-module/test-esm-loader-modulemap.js

-    message: 'The "job" argument must be of type ModuleJob. ' +
-             'Received type string'
+    message: 'The "job" argument must be an instance of ModuleJob. ' +
+             "Received type string ('notamodulejob')"


Micro-nit, but in messages like this one, I think it's more idiomatic for strings to be in double-quotes. Of course, both are fine in JavaScript so feel free to ignore this. People will know what is meant. (Only noticed it myself because job on the line above is in double quotes, but that's not even a string variable value, it's an identifier, so
¯\(ツ)/¯ but I did have to stop and wonder why one was in double-quotes and another in single-quotes.)

I rely upon .inspect() in this case and it's not possible to configure the quotation in that case. Therefore I'd stick to this for now (even though it would be possible to special handle strings).

This makes sure the original input is passed to the error in case no matching inputs are found. Instead of passing along all values, only valid or possibliy valid values are passed through. That way invalid values end up in the error case with the original input.

ERR_INVALID_ARG_TYPE is the most common error used throughout the code base. This improves the error message by providing more details to the user and by indicating more precisely which values are allowed ones and which ones are not. It adds the actual input to the error message in case it's a primitive. If it's a class instance, it'll print the class name instead of "object" and "falsy" or similar entries are not named "type" anymore.

Linter issue

nodejs-github-bot · 2019-12-20T00:22:41Z

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

This makes sure the original input is passed to the error in case no matching inputs are found. Instead of passing along all values, only valid or possibliy valid values are passed through. That way invalid values end up in the error case with the original input. PR-URL: #29675 Reviewed-By: Rich Trott <rtrott@gmail.com>

ERR_INVALID_ARG_TYPE is the most common error used throughout the code base. This improves the error message by providing more details to the user and by indicating more precisely which values are allowed ones and which ones are not. It adds the actual input to the error message in case it's a primitive. If it's a class instance, it'll print the class name instead of "object" and "falsy" or similar entries are not named "type" anymore. PR-URL: #29675 Reviewed-By: Rich Trott <rtrott@gmail.com>

BridgeAR · 2019-12-20T02:55:58Z

Landed in fc28761, ac2fc0d 🎉

This makes sure the original input is passed to the error in case no matching inputs are found. Instead of passing along all values, only valid or possibliy valid values are passed through. That way invalid values end up in the error case with the original input. PR-URL: #29675 Reviewed-By: Rich Trott <rtrott@gmail.com>

ERR_INVALID_ARG_TYPE is the most common error used throughout the code base. This improves the error message by providing more details to the user and by indicating more precisely which values are allowed ones and which ones are not. It adds the actual input to the error message in case it's a primitive. If it's a class instance, it'll print the class name instead of "object" and "falsy" or similar entries are not named "type" anymore. PR-URL: #29675 Reviewed-By: Rich Trott <rtrott@gmail.com>

This makes sure the original input is passed to the error in case no matching inputs are found. Instead of passing along all values, only valid or possibliy valid values are passed through. That way invalid values end up in the error case with the original input. PR-URL: #29675 Reviewed-By: Rich Trott <rtrott@gmail.com>

ERR_INVALID_ARG_TYPE is the most common error used throughout the code base. This improves the error message by providing more details to the user and by indicating more precisely which values are allowed ones and which ones are not. It adds the actual input to the error message in case it's a primitive. If it's a class instance, it'll print the class name instead of "object" and "falsy" or similar entries are not named "type" anymore. PR-URL: #29675 Reviewed-By: Rich Trott <rtrott@gmail.com>

This makes sure the original input is passed to the error in case no matching inputs are found. Instead of passing along all values, only valid or possibliy valid values are passed through. That way invalid values end up in the error case with the original input. PR-URL: #29675 Reviewed-By: Rich Trott <rtrott@gmail.com>

ERR_INVALID_ARG_TYPE is the most common error used throughout the code base. This improves the error message by providing more details to the user and by indicating more precisely which values are allowed ones and which ones are not. It adds the actual input to the error message in case it's a primitive. If it's a class instance, it'll print the class name instead of "object" and "falsy" or similar entries are not named "type" anymore. PR-URL: #29675 Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 23, 2019

Trott reviewed Sep 24, 2019

View reviewed changes

BridgeAR added the review wanted PRs that need reviews. label Sep 26, 2019

BridgeAR force-pushed the improve-ERR_INVALID_ARG_TYPE-2 branch from e5c6854 to 9284e5e Compare November 26, 2019 18:33

Trott reviewed Dec 19, 2019

View reviewed changes

Trott approved these changes Dec 19, 2019

View reviewed changes

BridgeAR added 3 commits December 19, 2019 19:27

fixup: use object instead of Object is most cases

b033324

BridgeAR force-pushed the improve-ERR_INVALID_ARG_TYPE-2 branch from 9284e5e to b033324 Compare December 19, 2019 18:58

This comment has been minimized.

Sign in to view

fixup: errors: improve ERR_INVALID_ARG_TYPE

0e7b9ff

Linter issue

This comment has been minimized.

Sign in to view

BridgeAR mentioned this pull request Dec 19, 2019

test: add coverage for vm: Module and SyntheticModule #31028

Closed

3 tasks

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 20, 2019

BridgeAR closed this Dec 20, 2019

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: improve ERR_INVALID_ARG_TYPE #29675

errors: improve ERR_INVALID_ARG_TYPE #29675

BridgeAR commented Sep 23, 2019

nodejs-github-bot commented Sep 23, 2019

Trott Sep 24, 2019 •

edited

BridgeAR Sep 24, 2019

vsemozhetbyt Sep 24, 2019 •

edited

BridgeAR Sep 24, 2019

Trott Sep 25, 2019

addaleax Sep 25, 2019

BridgeAR Sep 25, 2019

BridgeAR Sep 26, 2019

addaleax Sep 26, 2019

BridgeAR Sep 26, 2019 •

edited

Trott commented Oct 1, 2019

Trott Dec 19, 2019 •

edited

BridgeAR Dec 19, 2019

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Dec 20, 2019

BridgeAR commented Dec 20, 2019

errors: improve ERR_INVALID_ARG_TYPE #29675

errors: improve ERR_INVALID_ARG_TYPE #29675

Conversation

BridgeAR commented Sep 23, 2019

Checklist

nodejs-github-bot commented Sep 23, 2019

Trott Sep 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Sep 24, 2019 • edited

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 Sep 26, 2019 • edited

Choose a reason for hiding this comment

Trott commented Oct 1, 2019

Trott Dec 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Dec 20, 2019

BridgeAR commented Dec 20, 2019

Trott Sep 24, 2019 •

edited

vsemozhetbyt Sep 24, 2019 •

edited

BridgeAR Sep 26, 2019 •

edited

Trott Dec 19, 2019 •

edited