events: update and clarify error message#10387
Conversation
nodejs-github-bot added events 
cjihrig added the semver-major 
lib/events.js Outdated
lib/events.js Outdated
sam-github left a comment
sam-github left a comment There was a problem hiding this comment.
Unspecified is confusing, yes, but I don't think the text is an improvement. I can think of 2 other ways to avoid the abort that don't involve listening for the error (the 'uncaughtException' handler and domains), so the text is both too long, and not correct.
How about just 'Uncaught "error" event'?
ctide commented Dec 21, 2016
Fixed, sorry, didn't realize that running tests didn't run the linter as well. |
ctide commented Dec 21, 2016
Sure, that's reasonable. Unspecified was what really threw me off. I'll update and rebase. |
sam-github commented Dec 21, 2016
Also, needs a unit test - and yes, I know, it should have already had one, sorry! |
ctide commented Dec 21, 2016
Seemed like a message was the cleanest way to add a test for this. If that's not the right approach, let me know and I can update! |
There was a problem hiding this comment.
[object Object] is probably not the desired output.
cjihrig commented Dec 21, 2016
You don't need a message test. There is almost certainly a test that already exercises this behavior. You would just need to add an assertion on the error message in such a test. |
ctide commented Dec 21, 2016
OK, found the real test and just updated the regex to include the changes. |
lib/events.js Outdated
There was a problem hiding this comment.
I think either the parens should be dropped or the colon removed.
mscdex commented Dec 21, 2016
IMHO I think "uncaught" is a bit of a misnomer, since to me it sounds like the error was thrown because of a missing try-catch or something. What about "unhandled" instead? |
ctide commented Dec 21, 2016
Yeah, unhandled is more accurate than uncaught. Will push an update in a minute that changes it to that. |
There was a problem hiding this comment.
People should not be emitting strings or other things, even though we technically support it. So, in most cases, we'll print the rather unhelpful [object Object], like in your previous test, right?
There was a problem hiding this comment.
Certainly in all of my team's code, we pass an object to the emitter so it would just spit that out, yes.
There was a problem hiding this comment.
I'd opt to just not try to print the error. You could use util.format(), but it prints the whole stack trace for errors, so it's kind of loud.
There was a problem hiding this comment.
Yeah, I just omitted it and made the test reflect a real world use case slightly better.
ctide commented Dec 22, 2016
@Trott: I think maybe I was just on the wrong branch. It was my first time pulling down and running the node tests. I tried to recreate it to see if it was something with first time configuring & running tests, and it ran the linter. |
There was a problem hiding this comment.
Nit: how about using ^Error: Unhandled "error" event$? Pointing this out only because other tests do this.
joaocgreis commented Dec 23, 2016
Re-run of |
jasnell left a comment
jasnell left a comment There was a problem hiding this comment.
This LGTM but might have broader impact than expected. @nodejs/ctc thoughts?
lib/events.js Outdated
There was a problem hiding this comment.
I'm not sure it's a good idea to remove the last part. I understand that if you emit a plain object, it will print [object Object] but it seems logical for the "error" event to emit an Error and in this case it would print the error message correctly.
There was a problem hiding this comment.
yea, I would prefer that we keep the er in the message
lib/events.js Outdated
There was a problem hiding this comment.
Nit: Prefer const over var wherever possible.
jasnell commented Mar 22, 2017
@nodejs/ctc ... any further thoughts on this one? |
Update error message that's thrown when no error listeners are attached to an emitter. This would have been trivial to debug if I had understood what 'unspecified error' was supposed to mean. Once I went looking at the source, and saw the comment on 149, it was easy to fix.
ctide commented Mar 23, 2017
Rebased, and a few minor things based on the other feedback (adding er back, changing var to const, using fat arrow functions in the test instead.) |
jasnell commented Mar 23, 2017
@sam-github@cjihrig ... PTAL |
jasnell commented Mar 24, 2017
Update error message that's thrown when no error listeners are attached to an emitter. PR-URL: #10387 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Italo A. Casas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
jasnell commented Mar 24, 2017
Landed in 2141d37 |
13 participants
eventemitter: clarify error message
Update error message that's thrown when no error listeners are attached to an emitter. This would have been trivial to debug if I had understood what 'unspecified error' was supposed to mean. Once I went looking at the source, and saw the comment on 149, it was easy to fix.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
eventemitter
Description of change
Update error message that's thrown when no error listeners are attached to an emitter. This would have been trivial to debug if I had understood what 'unspecified error' was supposed to mean. Once I went looking at the source, and saw the comment on 149, it was easy to fix.