errors: improve ERR_INVALID_ARG_TYPE

[BridgeAR]

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]

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

[Trott]

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.

[BridgeAR]

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?

[vsemozhetbyt]

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.

[BridgeAR]

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

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

[addaleax]

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?

[BridgeAR]

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:

letinput={}// Validinput=/a/// Validinput=newTypeError()// Validinput=newMap()// Invalid

[BridgeAR]

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

[addaleax]

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.

[BridgeAR]

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]

@nodejs/collaborators This could use some reviews.

[Trott]

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

[BridgeAR]

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

[nodejs-github-bot]

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

[BridgeAR]

Landed in fc28761, ac2fc0d 🎉