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

errors: improve ERR_INVALID_ARG_TYPE #29675

Closed

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Sep 23, 2019

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

This comment has been minimized.

@@ -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);

This comment has been minimized.

Copy link
@Trott

Trott Sep 24, 2019

Member

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.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Sep 24, 2019

Author Member

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?

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Sep 24, 2019

Contributor

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.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Sep 24, 2019

Author Member

@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.

This comment has been minimized.

Copy link
@Trott

Trott Sep 25, 2019

Member

@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.

This comment has been minimized.

Copy link
@addaleax

addaleax Sep 25, 2019

Member

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?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Sep 25, 2019

Author Member

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

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Sep 26, 2019

Author Member

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

This comment has been minimized.

Copy link
@addaleax

addaleax Sep 26, 2019

Member

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.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Sep 26, 2019

Author Member

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

This comment has been minimized.

Copy link
Member

Trott commented Oct 1, 2019

@nodejs/collaborators This could use some reviews.

@BridgeAR BridgeAR force-pushed the BridgeAR:improve-ERR_INVALID_ARG_TYPE-2 branch from e5c6854 to 9284e5e Nov 26, 2019
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')"

This comment has been minimized.

Copy link
@Trott

Trott Dec 19, 2019

Member

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.)

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Dec 19, 2019

Author Member

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).

@Trott
Trott approved these changes Dec 19, 2019
BridgeAR added 3 commits Sep 23, 2019
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.
@BridgeAR BridgeAR force-pushed the BridgeAR:improve-ERR_INVALID_ARG_TYPE-2 branch from 9284e5e to b033324 Dec 19, 2019
@nodejs-github-bot

This comment was marked as outdated.

Linter issue
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

BridgeAR added a commit that referenced this pull request Dec 20, 2019
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>
BridgeAR added a commit that referenced this pull request Dec 20, 2019
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

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 20, 2019

Landed in fc28761, ac2fc0d 🎉

@BridgeAR BridgeAR closed this Dec 20, 2019
BridgeAR added a commit that referenced this pull request Jan 3, 2020
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>
BridgeAR added a commit that referenced this pull request Jan 3, 2020
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 BridgeAR mentioned this pull request Jan 7, 2020
targos added a commit that referenced this pull request Jan 14, 2020
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>
targos added a commit that referenced this pull request Jan 14, 2020
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>
targos added a commit that referenced this pull request Jan 14, 2020
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>
targos added a commit that referenced this pull request Jan 14, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.