meta: semver impact of Error messages

[refack]

• Version: master
• Platform: *
• Subsystem: errors

Context

There is an ongoing effort to migrate all JS expectations throws by node core to have a .code property. The error code allows userland to react in different exceptions specific ways.
To that extent a new internal module (internal/errors) was created #11220 and the migration is tracked in #11273 (and to less extent in https://github.com/nodejs/node/projects/4) Some module migrations landed in node@v8.0.0, some have landed in master since, and some have yet to be migrated. Hence it can't be asserted that node core will only throw errors with an error-code, and so we have to assume the existence of userland code that depends on error.message parsing.

Issue

At present there seems to be a difference of opinions as to the semver level of changes in messages of internal/errors.

Action item

A consensus should be reached on this issue as we want to have a clear path forward, and unambiguous guidelines for contributors.

Relevant discussion

1. guides/using-internal-errors.md
   

2. internal/errors: improve ERR_INVALID_ARG_TYPE #13730 (comment)
   

3. internal/errors: improve ERR_INVALID_ARG_TYPE #13730 (comment) (landed as semver-patch but marked don't land on v8.x until discoution is concluded)
   

4. errors: improve invalid arg type #13834 (comment)
   

[EDIT by @Trott: don't land -> don't land on v8.x for clarity.]

[addaleax]

Since this is tagged ctc-review, @nodejs/ctc

I don’t have a strong personal opinion about this, but I see @mscdex’ point, and I would be okay with considering these semver-major while Node 8 is Current (but trying to clearly message that that is going to change with the next major version).

[jasnell]

Generally I don't see the point in pushing it out further like that. Switching to the new mechanism is already semver-major which means the overwhelming majority of changes in this area already won't be in 8.x at any point in time. Those that are in 8.x were also semver-major changes that users will already be adapting to -- which involves moving away from checking the error message text in favor of checking the code. Requiring that these all be semver-major while 8.x is current won't provide any tangible benefit.

[Trott]

Should this be put on the LTS WG agenda? I'm OK with LTS being the final arbiter on what is and isn't a breaking change. (And if that isn't written into their charter, perhaps it ought to be?)

[Trott]

(Might also be good to clarify if LTS WG is the small-ish number of people listed in the LTS repo README or the considerably larger group of people on the @nodejs/lts team.)

[richardlau]

(Might also be good to clarify if LTS WG is the small-ish number of people listed in the LTS repo README or the considerably larger group of people on the @nodejs/lts team.)

See nodejs/Release#223

[benjamingr]

I'd like to see a citgm run - I think we can actually test if this must be semver major or not relatively quickly. I'm not sure on what PR I should run the citgm run against though - would love assistance with that.

[refack]

CITGM for #13834: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/891/
CITGM on master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/892/
CITGM for #13834: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/893/

[mscdex]

I think we can actually test if this must be semver major or not relatively quickly

IMHO this is one of those things where end user code could be just as easily affected as modules on npm.

[benjamingr]

@mscdex my point was that if CITGM is red (and it is red) we have to mark it semver-major (and we should). Since it breaks stuff in the ecosystem we can be sure it breaks user code.

We should also probably notify those 50 packages in NPM about the breakage.

[mhdawson]

I'm thinking we need a communications push along with some period of time for people to adjust. Ideally we'd have all of the messages moved over and then do a fair bit of communications/evangelism. Its going to be easier to say it applies to all messages than explaining a different policy based on the message.

For messages that we have moved over (semver-major) and are changing again the main question to me is if there has been enough communication/knowledge transfer so that developers know that they should no longer be checking the error message and instead use the error code so that they don't get affected by the second change. Until then I was thinking it would be safer to continue to mark them as semver-major.

[mcollina]

@mhdawson it will take some time. Module author needs to adjust, and old versions of Node.js phase out. I would say we should wait until Node 10 goes LTS, i.e. we might stop considering these changed after 2 current releases (one of which LTS) with the new system.