New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: update and clarify error message #10387

Closed
wants to merge 1 commit into
base: master
from

Conversation

@ctide
Contributor

ctide commented Dec 21, 2016

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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected 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.

Show outdated Hide outdated lib/events.js
Show outdated Hide outdated lib/events.js
@sam-github

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

This comment has been minimized.

Show comment
Hide comment
@ctide

ctide Dec 21, 2016

Contributor

Fixed, sorry, didn't realize that running tests didn't run the linter as well.

Contributor

ctide commented Dec 21, 2016

Fixed, sorry, didn't realize that running tests didn't run the linter as well.

@ctide

This comment has been minimized.

Show comment
Hide comment
@ctide

ctide Dec 21, 2016

Contributor

Sure, that's reasonable. Unspecified was what really threw me off. I'll update and rebase.

Contributor

ctide commented Dec 21, 2016

Sure, that's reasonable. Unspecified was what really threw me off. I'll update and rebase.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Dec 21, 2016

Member

Also, needs a unit test - and yes, I know, it should have already had one, sorry!

Member

sam-github commented Dec 21, 2016

Also, needs a unit test - and yes, I know, it should have already had one, sorry!

@ctide

This comment has been minimized.

Show comment
Hide comment
@ctide

ctide Dec 21, 2016

Contributor

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!

Contributor

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!

Show outdated Hide outdated test/message/unhandled_eventemitter_exception.out
@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 21, 2016

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ctide

ctide Dec 21, 2016

Contributor

OK, found the real test and just updated the regex to include the changes.

Contributor

ctide commented Dec 21, 2016

OK, found the real test and just updated the regex to include the changes.

@mscdex mscdex removed the lts-watch-v6.x label Dec 21, 2016

Show outdated Hide outdated lib/events.js
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Dec 21, 2016

Contributor

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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ctide

ctide Dec 21, 2016

Contributor

Yeah, unhandled is more accurate than uncaught. Will push an update in a minute that changes it to that.

Contributor

ctide commented Dec 21, 2016

Yeah, unhandled is more accurate than uncaught. Will push an update in a minute that changes it to that.

Show outdated Hide outdated test/parallel/test-event-emitter-errors.js
@cjihrig

One nit, but then this looks good to me.

Show outdated Hide outdated test/parallel/test-event-emitter-errors.js
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Dec 21, 2016

Member

Fixed, sorry, didn't realize that running tests didn't run the linter as well.

It does (unless there is a failing test in which case make/vcbuild exit before it gets to the linter).

If it didn't run for you after a successful test run, that's really strange and I'd be interested to know if there was something at all unusual about the way you ran the tests.

Member

Trott commented Dec 21, 2016

Fixed, sorry, didn't realize that running tests didn't run the linter as well.

It does (unless there is a failing test in which case make/vcbuild exit before it gets to the linter).

If it didn't run for you after a successful test run, that's really strange and I'd be interested to know if there was something at all unusual about the way you ran the tests.

@ctide

This comment has been minimized.

Show comment
Hide comment
@ctide

ctide Dec 22, 2016

Contributor

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

Contributor

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.

Show outdated Hide outdated test/parallel/test-event-emitter-errors.js
@lpinca

lpinca approved these changes Dec 22, 2016

@italoacasas

This comment has been minimized.

Show comment
Hide comment
@joaocgreis

This comment has been minimized.

Show comment
Hide comment
@joaocgreis

joaocgreis Dec 23, 2016

Member

Re-run of arm-fanned because of the RPi1 failures: https://ci.nodejs.org/job/node-test-commit-arm-fanned/6082/

Member

joaocgreis commented Dec 23, 2016

Re-run of arm-fanned because of the RPi1 failures: https://ci.nodejs.org/job/node-test-commit-arm-fanned/6082/

@jasnell

This LGTM but might have broader impact than expected. @nodejs/ctc thoughts?

@mscdex mscdex removed the lts-watch-v6.x label Dec 23, 2016

@@ -160,7 +160,7 @@ EventEmitter.prototype.emit = function emit(type) {
throw er; // Unhandled 'error' event
} else {
// At least give some kind of context to the user
var err = new Error('Uncaught, unspecified "error" event. (' + er + ')');

This comment has been minimized.

@targos

targos Dec 26, 2016

Member

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.

@targos

targos Dec 26, 2016

Member

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.

This comment has been minimized.

@mscdex

mscdex Mar 22, 2017

Contributor

I agree.

@mscdex

mscdex Mar 22, 2017

Contributor

I agree.

This comment has been minimized.

@evanlucas

evanlucas Mar 22, 2017

Member

yea, I would prefer that we keep the er in the message

@evanlucas

evanlucas Mar 22, 2017

Member

yea, I would prefer that we keep the er in the message

Show outdated Hide outdated lib/events.js
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 22, 2017

Member

@nodejs/ctc ... any further thoughts on this one?
@ctide ... if you'd like to pursue this can you please give it a quick rebase?

Member

jasnell commented Mar 22, 2017

@nodejs/ctc ... any further thoughts on this one?
@ctide ... if you'd like to pursue this can you please give it a quick rebase?

Update events.js
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

This comment has been minimized.

Show comment
Hide comment
@ctide

ctide Mar 23, 2017

Contributor

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

Contributor

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

@targos

targos approved these changes Mar 23, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Mar 23, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment

jasnell added a commit that referenced this pull request Mar 24, 2017

events: update and clarify error message
Update error message that's thrown when no error listeners are attached
to an emitter.

PR-URL: #10387
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 24, 2017

Member

Landed in 2141d37

Member

jasnell commented Mar 24, 2017

Landed in 2141d37

@jasnell jasnell changed the title from Update events.js to events: update and clarify error message Mar 24, 2017

@jasnell jasnell closed this Mar 24, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment