errors: change ERR_HTTP2_HEADER_SINGLE_VALUE to instance of TypeError

[davidmarkclements]

changes the base instance for ERR_HTTP2_HEADER_SINGLE_VALUE
from Error to TypeError as a more accurate representation
of the error. Additionally corrects the grammar of the error
message.

Arguably the error type could also be RangeError, but only in cases
where a header key in an object is assigned an array with more than one
element, instead of a string. However, this inconsistency could be confusing
and unnecessarily complex/convoluted. It could also be (possibly weakly) argued
that a "type" in JS is defined by object shape, so an object that's an instance of Array,
with one key (0) is the expected type.

Checklist

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

[BridgeAR]

LGTM if CI is green.

[BridgeAR]

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

[Trott]

Maybe (probably?) I misunderstand something, but if I understand correctly, then I disagree that this is a TypeError.

If you expect an array and you get a string, that's a TypeError.

If you get an array with four values, but you expect an array with 1 value, that is not a TypeError.

If you expect an object with certain keys, but get an object with additional unexpected keys, that is also not a TypeError.

[Trott]

Although the definition of TypeError in the current draft ECMAScript spec certainly suggests that I'm wrong and this could be considered a TypeError...

https://tc39.github.io/ecma262/#sec-native-error-types-used-in-this-standard-typeerror:

TypeError is used to indicate an unsuccessful operation when none of the other NativeError objects are an appropriate indication of the failure cause.

[Trott]

But that's a draft. :-D Current spec is more aligned with what I'm saying...

https://www.ecma-international.org/ecma-262/6.0/#sec-native-error-types-used-in-this-standard-typeerror

Indicates the actual type of an operand is different than the expected type.

¯\(ツ)/¯

[Trott]

pinging people that have a reason to read/understand the spec and might have an opinion one way or the other: @TimothyGu @nodejs/v8

[BridgeAR]

We actually do not always adhere to the spec. Neither do the browsers. It is often used in general if the input is not as expected.

[davidmarkclements]

there are two cases where this error occurs:

1. Where an array is supplied with more than one element (https://github.com/nodejs/node/blob/master/lib/internal/http2/util.js#L435)
2. Where a string is supplied multiple times (https://github.com/nodejs/node/blob/master/lib/internal/http2/util.js#L442)

I think it could be indeed be argued that this is a RangeError - depending
on whether 2 could be classified as a RangeError. In either TypeError or RangeError
case you have to stretch the concept a little bit.

[Trott]

What's the benefit of having this be a TypeError (or RangeError, which I agree also does not quite fit but works if you squint a little) instead of a generic Error?

[davidmarkclements]

classification accuracy.

but granted, if it's agree it doesn't fit in any classification,
this PR will at least address the comment suggesting that it's a TypeError

[Trott]

this PR will at least address the comment suggesting that it's a TypeError

I'd suggest simply removing that comment. :-D

[BridgeAR]

@Trott as I pointed out: this is actually in line with the way errors are mainly implemented in core. A generic Error is mainly the cause if something internal / deep down does not work right. A TypeError immediately tells the user that it is about the input value.

[davidmarkclements]

happy to change as necessary

@jasnell - with your familiarity with HTTP2, what's your thoughts?

=== edit - you're -> your

[jasnell]

I believe TypeError is appropriate here based on general convention.

[Trott]

A generic Error is mainly the cause if something internal / deep down does not work right.

I see the value of "this is something you did in your code, and not a problem with Node.js". But I don't think this generalization is correct. (ERR_ASSERTION, ERR_CANNOT_WATCH_SIGINT, ERR_CHILD_CLOSED_BEFORE_REPLY, and many many many more...)

Moreover, I think the basic suggestion of all those comments in errors.js is "Invalid values should be a TypeError" and I think that is wrong and we should not do that.

[Trott]

I believe TypeError is appropriate here based on general convention.

@jasnell Can you explain the "general convention" that you are referring to? Only thing I can come up with would be "Invalid input should automatically be treated as a TypeError if it cannot be treated as a RangeError", and I can't get behind that....

[jasnell]

I'm happy with this change but I'm not passionate enough about the exact semantics of TypeError to put much time into arguing the case.

[BridgeAR]

@Trott about the convention: if you look at all the errors in internal/errors and check the errors that are meant to be a TypeError you are going to stumble upon multiple entries that only check for "invalid input" and not for "wrong type".

So e.g., ERR_ASYNC_TYPE (it will be thrown in case a string is empty).
ERR_CRYPTO_ECDH_INVALID_FORMAT
ERR_CRYPTO_INVALID_DIGEST
ERR_ENCODING_INVALID_ENCODED_DATA
ERR_HTTP2_ALTSVC_INVALID_ORIGIN
ERR_HTTP2_ALTSVC_LENGTH (should probably be a RangeError)
ERR_HTTP2_INVALID_HEADER_VALUE (this is on the edge out of my perspective)
ERR_INVALID_FD_TYPE
.... and so on (I stopped checking).

Almost all TypeError that are checked fall in that category. So that is the reason for the general convention.

[davidmarkclements]

So then... do we

• change many of the existingTypeError to Error
• update remaining user input from Error to TypeError
• leave as is (inconsistent)

[Trott]

@BridgeAR Well, that list is pretty hard to argue with. I would prefer we go the other direction and make those things (that are already TypeError instances that really are just validating input content rather than type) Error instances rather than the other way around, but I certainly agree that consistency is an improvement. I'll remove my request for changes. Not excited about seeing all these happen, but also not going to resist (unless there are others more perturbed by it than I am).

(For what it's worth, my thinking is: Any error is DEFINITELY an Error. So if something might or might not be a TypeError, go with the more generic Error and save TypeError for things that are unquestionably TypeErrors. By using TypeError for things that aren't in fact TypeErrors, we make the general use of TypeError less valuable because the user is less sure what it means when it appears.)

At least it's consistent...

[mcollina]

LGTM

[mcollina]

Landed as ef07d65.

[BridgeAR]

Since HTTP2 is still experimental, it actually does not have to be semver-major.