meta: semver impact of Error messages #13937

refack · 2017-06-26T20:31:20Z

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 [email protected], 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

guides/using-internal-errors.md
internal/errors: improve ERR_INVALID_ARG_TYPE #13730 (comment)
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)
errors: improve invalid arg type #13834 (comment)

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

The text was updated successfully, but these errors were encountered:

addaleax · 2017-06-26T20:39:37Z

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 · 2017-06-26T20:44:59Z

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 · 2017-06-27T03:08:18Z

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 · 2017-06-27T03:10:46Z

(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 · 2017-06-27T06:36:58Z

(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 · 2017-06-27T15:35:50Z

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 · 2017-06-27T16:00:40Z

~~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 · 2017-06-27T17:09:03Z

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 · 2017-06-27T22:25:30Z

@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 · 2017-06-28T13:08:11Z

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 · 2017-06-28T13:13:48Z

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

refack · 2017-06-28T23:09:22Z

So anyway I marked #13730 that's already in master dont-land-on-v8.x

mhdawson · 2017-06-30T16:23:16Z

@mcollina I agree it might take some time. I don't think we need to decide when we might be ready, but we do want agreement on whether we are not ready yet and should continue to mark them SemVer major for now.

mcollina · 2017-06-30T17:34:47Z

@nodejs/ctc do we want to talk about this in the next meeting?

jasnell · 2017-06-30T18:16:13Z

I've tagged with ctc-agenda

Trott · 2017-07-15T12:56:14Z

@nodejs/ctc Currently, this is the only issue for the meeting this week. If I'm understanding everyone's text and GitHub reaction emoji things correctly, expressed opinions thus far are:

CTC folks

these changes are semver-major for the time being: @mscdex @mhdawson @mcollina
no strong opinion but I can see why they might still be considered semver-major: @addaleax
not semver-major: @jasnell @cjihrig

Other Collaborators and Contributors

not semver-major: @refack @BridgeAR
whether or not these sorts of changes should be considered semver major may depend on individual CITGM runs for each change: @benjamingr

Anyone else care to weigh in? @nodejs/collaborators

As of now, we don't have consensus and I don't think we are likely to get consensus before the meeting. If I had to guess, I'd say this is going to end up going to a CTC vote. I'd therefore ask that @nodejs/ctc members decide that either they are OK with any result or else invest whatever time is needed to come to an opinion before the meeting this week.

Of course, you may change your mind during the meeting based on conversation etc. But if it comes to a vote, I would prefer we actually have resolution on the vote quickly.

Trott · 2017-07-15T13:19:43Z

My opinion: While I really want message changes to not be semver-major anymore, the fact is our own docs say:

in onboarding.md: "If a change has the remote chance of breaking something, use the semver-major label"
and again in onboarding-extras.md: "be conservative – that is, if a change has the remote chance of breaking something, go for semver-major"

True, it says in the "Using Internal Errors" guide that "once using internal/errors, changes to internal/errors error messages will be handled as semver-minor or semver-patch."

However, by definition, guides are more advisory and less binding than other docs. (I know that I certainly review all CTC governance doc changes very closely while I tend to be less granular in my reviews for guides unless the guide happens to be on a topic I am extremely interested in. And I suspect I'm not the only person for whom that's true.)

When a guide contradicts our governance docs or other docs, the contradictory element of the guide should be disregarded at least until the docs are updated. (I'm using "should" and not "must" very deliberately in that previous sentence.)

There is clearly at least a "remote chance of breaking something" with message changes. That's why we're having this conversation.

Therefore, these sorts of changes should be treated as semver-major at the current time.

Response to some obvious objections:

This line of reasoning all becomes invalid as soon as someone PRs and lands changes to those two docs.
- My response: I'm honestly not sure a change that invalidates this line of thinking completely would pass without objection. You can probably change "remote chance" to "reasonable chance" but that doesn't invalidate my line of thinking here.

We have treated things that have a "remote chance of breaking something" as not-semver-major before.
- My response: Just because we did something incorrectly in the past is not (in this case, anyway) a reason to continue doing something incorrectly.

Really, nearly any change has a "remote chance of breaking something". A literal interpretation of the wording is unworkable.
- My response: The reasoning above all holds up without a hyper-literal interpretation of "remote chance". Treating "remote chance" as "reasonable chance" doesn't change anything here.

refack · 2017-07-15T13:20:43Z

I had an idea for a middle ground solution. Declare that the move to the new Errors and their messages are experimental for the pre-v9.0 timeframe, and changes carry a blanket do-not-land-on-*.
The motivation is to enable the Contributors the freedom to work on this without the need to involve CTC members for every comma moved.

Trott · 2017-07-15T13:23:12Z

The motivation is to enable the Contributors the freedom to work on this without the need to involve CTC members for every comma moved.

Has getting sufficient CTC approval on message changes been a significant problem? (Honest question. I don't know the answer.)

refack · 2017-07-15T15:32:53Z

Has getting sufficient CTC approval on message changes been a significant problem? (Honest question. I don't know the answer.)

A bit.
#14057 has been open for 25 days (spin off of #13834)
#13976 was open for 13 days
#11384 Opened on 2/14/2017
#13994 Open for 16 days
#13476 Opened on 5/6 (still needs one more reviewer)
#13829 been open 25 days
And the one that triggered this thread #13730 landed without 2 CTC approvals.
Since most of those are "busy-work" IMHO they should not be encumbered by the semver-major policy, since AFAIK the move to internal/Error has general approval. They just should not land in v8.x....

Trott · 2017-07-15T16:17:56Z

@refack Thanks for compiling that list.

A few things:

#14057 has been open for 25 days (spin off of #13834)

Correction: That PR itself has only been open for 12 days and had two CTC approvals after 9 days.

More importantly, two things:

First, I was asking specifically about PRs for message changes. Most of these are about initial implementation of the internal errors stuff so will be subject to the two-CTC-approvals rule no matter what we decide on this issue. Simple message changes should be easy to review and thus likely incur a smaller cost in terms of delay waiting for two CTC approvals.

Second, I think the useful metric here is the time difference between when a PR could have landed if it wasn't semver-major vs. how long it took as a semver-major.

So let's remove the ones that were about more than straightforward changing the text of an error message or two. We're left with only one PR from that list:

internal/errors: improve ERR_INVALID_ARG_TYPE #13730 was delayed, I would argue, 0 days. It was going on 6 days delay when it landed, but for 3 of those days, it was blocked from landing anyway by a collaborator request for changes and a block label. During that time, @jasnell commented on it saying that it wasn't semver-major but not leaving an approval, quite possibly because he already saw it had amassed four approvals and that seemed like plenty. That comment from @jasnell came before the block was lifted, so his approval wouldn't have made it land-able anyway. But his approval would have been the second CTC approval. So semver-major or not, it (I would argue, at least) could have landed (if there had been consensus that it was in fact semver-major) at exactly the same time it became available for landing as a non-semver-major, which is when the block was removed.

That said, that is only one data point and I wouldn't want to draw too sweeping a conclusion (or, really, any conclusion) from it. I'd say at this point that we don't really know if keeping message changes as semver major is going to greatly delay PRs from landing.

Probably the right place to look for more data (if anyone is so inclined!) would be message changes on old-style errors. There is unanimity that those should be semver-major so that will tell us what the cost is rather than incorporate the cost of uncertainty and in-tracker debate about it.

mcollina · 2017-07-19T08:22:42Z

i'm not sure why it would be a mess; it'd be a static nonchanging map for any non-LTS versions, and LTS versions would be relatively minor to maintain. I'd be happy to volunteer to maintain it going forward if anyone's afraid of keeping things working on older nodes; it's not actually that difficult in practice, in my experience.

My experience maintaining a module across a wide range of Node.js versions is that it gets extremely complicated over time as core move faster than the community. Look at the rig we have to maintain to make readable-stream work from Node 0.6+ (https://github.com/nodejs/readable-stream/tree/master/build). It is a lot of work, and everyone just expect it to "just work" with 50 millions of monthly downloads. Eventually, it became extremely hard to make effective change.
If we have a solution that does not require such a module to exist, I would prefer it. That being said, it seems there is no other alternative. Have a look at https://github.com/jasnell/internal-errors from @jasnell to help readable-stream.

If that initiative starts, I think both myself and @calvinmetcalf should be on the team, as we might need something clever there to support streams.

refack · 2017-07-19T12:36:05Z

That way, you could publish that package - the transition tool - independently of node core, asap. Users could proactively refactor to use it (even if, like me, they support node 0.6), and then future message changes would not break them (so users are incentivized to use this package). The new "best practice" could be to always use require('node-error-codes'), pass error instances into that function, and then use the code it returns (the package could also export code constants, so people don't have to hardcode them).

Not so ASAP since we didn't finish mapping all the errors 😞. But otherwise sounds like a great idea.

Trott · 2017-07-19T16:31:42Z

At the CTC meeting today, consensus was these message changes are still semver-major for the time being.

The issue of difficulty getting two CTC approvals came up and there was some discussion around how we might address that. /cc @MylesBorins @jasnell

refack · 2017-07-19T16:32:07Z

CTC (and this thread)'s decision:
Error message change are still semver-major.
Will be reviseted after the completion of migration and the release of v9

refack · 2017-07-19T16:33:55Z

If landing time will be an issue I'll open a new thread.

ChALkeR · 2017-07-19T17:20:05Z

At the CTC meeting today, consensus was these message changes are still semver-major for the time being.

One thing to note: once we declare that policy (which we in fact did here), changing that policy itself would be a semver-major imo.

ljharb · 2017-07-19T17:40:49Z

Was there any discussion of my suggestion for a cross-node core-included error code package?

Trott · 2017-07-19T17:54:06Z

Was there any discussion of my suggestion for a cross-node core-included error code package?

@ljharb No. TBH I encouraged us to make a decision on whether these are breaking changes, and to discuss other issues (2-CTC approval issues, how to get the new errors implemented faster, etc.) in GitHub issues.

joyeecheung · 2017-07-19T18:27:55Z

@ljharb I believe @jasnell talked about making the internal error system an npm package that readable-stream can depend on?

EDIT: oh I see it is already mentioned in #13937 (comment), it currently needs a bit of work to work on older versions of node.

refack · 2017-07-19T18:45:50Z

Was there any discussion of my suggestion for a cross-node core-included error code package?

AFAICT That's orthogonal. Maybe if it's up and running and adopted, we could trigger a reevaluation of the severity policy.

P.S. I have been thinking of way to assemble a map of error messages to error code, efficiently, and across release line... Not so trivial 🤔

PR-URL: nodejs#14375 Refs: nodejs#13937 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: #14375 Refs: #13937 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

jondubois · 2017-08-04T11:10:11Z

I read in this article https://medium.com/the-node-js-collection/node-js-errors-changes-you-need-to-know-about-dc8c82417f65 that there is a plan to change the error.name to contain the 'code' as part of the name string like CustomErrorName [ERROR_CODE]. I think this is a bad idea because:

A common strategy for handling errors up until now involve setting a custom name before throwing the error and then later checking the error name with if (err.name == 'NotAuthorizedError'); if we start adding error codes as part of the name string like NotAuthorizedError [SOME_ERROR_CODE] then it would break backwards compatibility with respect to that error handling strategy.
Adding the error code to the error name would be different from how the JavaScript error.name appears in the browser which does not contain any error code or brackets as part of the error name. Being as consistent with browsers as possible is a good thing.

refack · 2017-08-04T13:27:50Z

there is a plan to change the error.name to contain the 'code' as part of the name string like CustomErrorName [ERROR_CODE]. I think this is a bad idea...

@jondubois valid comment, so just to be clear this change will only affect Errors that node core it throwing. AFAIK there is no plan to touch "userland" Errors in any way.
For example if you did:

dgram.createSocket('i like rabbits')

core would throw

new Error('Bad socket type specified. Valid types are: udp4, udp6');

now instead the Error will have a code and it's name will include it:

{
   code: 'ERR_SOCKET_BAD_TYPE',
   type: Error,
   message: /^Bad socket type specified\. Valid types are: udp4, udp6$/
   name: `Error [ERR_SOCKET_BAD_TYPE]`
}

felixfbecker · 2018-04-24T17:41:27Z

I agree that adding the code to the name is weird. For me name is a programmatically-inspectable property, just like code, and not for "human consumption" like message. Having pretty formatting in there with a space and brackets goes against that.
Browser errors have no code property, they always use name, so that's where this intuition is coming from. For example, for fetch you would have to check if err.name === 'AbortError'. The only other option is using instanceof, but that is bad practice (assert on interfaces, not implementations).

Personally I see code as a subcategory of name, e.g. name can be TypeError and code ERR_SOCKET_BAD_TYPE. It's both useful to be able to make assertions about the broader category of and error as well as the exact error reason. For example, in an Express error handler, I might want to never return any TypeErrors to the client because they are programming errors. Or take p-retry as an example (emphasis mine):

Returns a Promise that is fulfilled when calling input returns a fulfilled promise. If calling input returns a rejected promise, input is called again until the max retries are reached, it then rejects with the last rejection reason.
It doesn't retry on TypeError as that's a user error.

Checking code here wouldn't be helpful.

I would prefer if both name and code are for programmatic inspection without fancy formatting, and Error.prototype.toString() could be modified to include both error.name and error.code in the output, as toString() is unarguably purely for human consumption. Doing this for all errors, including user errors, would be a great feature and encourage users to make use of error codes too. Alternatively it could only be an internal base error class.

haykam821 · 2018-05-03T23:36:17Z

encourage users to make use of error codes too

Please, allow us to set error.code as well as the message from the constructor if you want to do this.

throw new Error("Message", "ERR_CODE");

ljharb · 2018-05-03T23:41:26Z

@haykam821 that's part of the JS language itself, and is not something node should do. You can do Object.assign(new Error('message'), { code: 'ERR_CODE' }) if you need to.

haykam821 · 2018-05-03T23:46:21Z

@ljharb Yes

Ginden · 2018-05-04T12:35:09Z

Though, Node can (and in my opinion - should) expose helper function for creating errors with code.

jasnell · 2018-05-04T12:36:39Z

Eventually, perhaps. For now the focus is on making sure all of node.js' own errors have assigned codes and consistent error messages

refack added ctc-review errors Issues and PRs related to JavaScript errors originated in Node.js core. meta Issues and PRs related to the general management of the project. labels Jun 26, 2017

refack mentioned this issue Jun 26, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-06-28 nodejs/CTC#148

Closed

jasnell added the ctc-agenda label Jun 30, 2017

Trott mentioned this issue Jul 2, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-07-05 nodejs/CTC#153

Closed

Trott removed the ctc-review label Jul 2, 2017

This was referenced Jul 5, 2017

test: refactor expectsError #14058

Closed

buffer: remove MAX_SAFE_INTEGER check on length #14131

Closed

refack mentioned this issue Jul 19, 2017

internal: use internal/errors #13829

Closed

3 tasks

Trott closed this as completed Jul 19, 2017

Trott removed the ctc-agenda label Jul 19, 2017

refack mentioned this issue Jul 19, 2017

doc: error message are still major #14375

Merged

2 tasks

refack mentioned this issue Aug 4, 2017

Tracking Issue: Node.js 9.0.0 #14391

Closed

stouf mentioned this issue Aug 24, 2017

Error.name vs Error.code nodejs/help#789

Closed

Trott mentioned this issue Apr 24, 2018

Error.name should not contain display elements #20253

Closed

azu mentioned this issue May 1, 2018

Incompatibility between Node.js core libs and webpack's lib webpack/node-libs-browser#78

Open

meta: semver impact of Error messages #13937

meta: semver impact of Error messages #13937

Comments

refack commented Jun 26, 2017 • edited by Trott Loading

Context

Issue

Action item

Relevant discussion

addaleax commented Jun 26, 2017

jasnell commented Jun 26, 2017

Trott commented Jun 27, 2017

Trott commented Jun 27, 2017

richardlau commented Jun 27, 2017

benjamingr commented Jun 27, 2017

refack commented Jun 27, 2017 • edited Loading

mscdex commented Jun 27, 2017

benjamingr commented Jun 27, 2017

mhdawson commented Jun 28, 2017

mcollina commented Jun 28, 2017

refack commented Jun 28, 2017

mhdawson commented Jun 30, 2017

mcollina commented Jun 30, 2017

jasnell commented Jun 30, 2017

Trott commented Jul 15, 2017 • edited Loading

CTC folks

Other Collaborators and Contributors

Trott commented Jul 15, 2017 • edited Loading

refack commented Jul 15, 2017

Trott commented Jul 15, 2017

refack commented Jul 15, 2017

Trott commented Jul 15, 2017 • edited Loading

mcollina commented Jul 19, 2017

refack commented Jul 19, 2017

Trott commented Jul 19, 2017

refack commented Jul 19, 2017

refack commented Jul 19, 2017

ChALkeR commented Jul 19, 2017 • edited Loading

ljharb commented Jul 19, 2017

Trott commented Jul 19, 2017

joyeecheung commented Jul 19, 2017 • edited Loading

refack commented Jul 19, 2017

jondubois commented Aug 4, 2017 • edited Loading

refack commented Aug 4, 2017

felixfbecker commented Apr 24, 2018 • edited Loading

haykam821 commented May 3, 2018

ljharb commented May 3, 2018

haykam821 commented May 3, 2018

Ginden commented May 4, 2018

jasnell commented May 4, 2018

refack commented Jun 26, 2017 •

edited by Trott

Loading

refack commented Jun 27, 2017 •

edited

Loading

Trott commented Jul 15, 2017 •

edited

Loading

Trott commented Jul 15, 2017 •

edited

Loading

Trott commented Jul 15, 2017 •

edited

Loading

ChALkeR commented Jul 19, 2017 •

edited

Loading

joyeecheung commented Jul 19, 2017 •

edited

Loading

jondubois commented Aug 4, 2017 •

edited

Loading

felixfbecker commented Apr 24, 2018 •

edited

Loading