I'm not for or against this, but it definitely defeats the purpose of the error code movement. Theoretically, the idea is that the error message strings can change freely but the codes themselves will never change.

Theoretically, the idea is that the error message strings can change freely but the codes themselves will never change.

@cjihrig Agreed. But I think the reduction of error codes is also important.

@cjihrig as far as I see it there is still going to be some work about that but it is going to reduce a lot over time.

I mark this as ready even though it should probably wait the full 48 hours and how do we handle error codes at the moment? Do we still require two TSC votes on it? I remember that we spoke about loosening that in Vancouver but I am not sure what happened afterwards about it.

changing error codes is definitely server-Major and would require two TSC votes

I haven't reviewed the code but I'm +1 on the change

Should this breaking change be documented somewhere, maybe the ERR_OUT_OF_RANGE should remain in doc/api/errors.md with a note saying it has been replaced, for those still expecting it?

Can we use ERR_OUT_OF_RANGE instead going forward? There are 4 places currently that have ERR_VALUE_OUT_OF_RANGE compared to like 35 uses of the former.

@apapirovski Actually there are 5 places using ERR_VALUE_OUT_OF_RANGE and 9 places using ERR_OUT_OF_RANGE in lib

We use ERR_VALUE_OUT_OF_RANGE instead of the other because the error message of ERR_VALUE_OUT_OF_RANGE is more detailed. It gives the expected range and received value in error message. In contrast, the ERR_OUT_OF_RANGE only gives the value name.

Also, while at it, could you update to use RangeError everywhere instead of TypeError?

Yes, I will fix it : )

Actually there are 5 places using ERR_VALUE_OUT_OF_RANGE and 9 places using ERR_OUT_OF_RANGE in lib

There are 5 total instances of ERR_VALUE_OUT_OF_RANGE, that is 5 total errors that throw that code. There are 37 instances of ERR_OUT_OF_RANGE, 28 in lib/fs.js alone.

We use ERR_VALUE_OUT_OF_RANGE instead of the other because the error message of ERR_VALUE_OUT_OF_RANGE is more detailed. It gives the expected range and received value in error message. In contrast, the ERR_OUT_OF_RANGE only gives the value name.

The error message can be modified on ERR_OUT_OF_RANGE instead. That's the whole point of error codes.

I would rather use the ERR_OUT_OF_RANGE code also.

I think I’d prefer ERR_VALUE_OUT_OF_RANGE, just because it seems not too unlikely that at some point we might want to have something like ERR_INDEX_OUT_OF_RANGE as well? And even if not, the ERR_VALUE_OUT_OF_RANGE doesn’t have that value/index ambiguity in its name.

Edit: Okay, in that case we might also want to call it “out of bounds”, not “out of range”… I still feel like that’s something that might be easy to mix up for non-native speakers of English

For ERR_INDEX_OUT_OF_RANGE, I would think we would want to just use ERR_OUT_OF_RANGE for both and just have the message say index is out of range or something. I'm not sure I see a benefit to using a unique error code for it.

The benefit of changing ERR_VALUE_OUT_OF_RANGE > ERR_OUT_OF_RANGE is:

1. less churn
2. while this remain semver-major, less people are potentially affected by this change.

IMO the introduction of ERR_VALUE_OUT_OF_RANGE was an oversight anyway. We had ERR_OUT_OF_RANGE much longer.

Pushed commit to address comment. Changes are:

• use ERR_OUT_OF_RANGE
• use RangeError everywhere
• document in doc/api/errors.md that ERR_VALUE_OUT_OF_RANGE is longer used and replaced by ERR_OUT_OF_RANGE.

Also, this could potentially land in two separate commits:

1. Improve error message for ERR_OUT_OF_RANGE (this can then be backported)
2. Change ERR_VALUE_OUT_OF_RANGE > ERR_OUT_OF_RANGE (semver-major)

I don't know if it's worth it but something to consider?

I'm not for or against this, but it definitely defeats the purpose of the error code movement. Theoretically, the idea is that the error message strings can change freely but the codes themselves will never change.

I'm not sure about that. The purpose of the movement was to allow the messages to be semver-patch. Actually then changing the code-meaning of the error is still semver-major just as normal.

Actually then changing the code-meaning of the error

I don't think that's happening in this PR, is it? The meaning is still exactly the same, just with more insight.

Rebased to resolve conflicts and fixed some broken tests.

This needs another rebase.

Rebased to resolve conflicts.

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

Rebased again to resolve conflicts.
Is there anyone can open a new CI and land this PR?

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

Landed in d022cb1