Skip to content

Conversation

@lpinca
Copy link
Member

@lpincalpinca commented Nov 9, 2017

I think we should not mention the 'uncaughtException' event in the documentation of the error events. Experience has taught me that it is usually used in the wrong way leading to bizarre situations like parsers blocked in a wrong state, events emitted multiple times when they should not, etc. If an 'error' event does not have a listener the process should just exit.

Checklist
Affected core subsystem(s)

doc

Avoid suggesting using `'uncaughtException'` for emitted errors.
@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. labels Nov 9, 2017
Copy link
Member

@bnoordhuisbnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already a step up so +1 from me.

@a0viedo
Copy link
Member

I'm not in favor of removing documentation if it's still supported but here's my concern: if user space is misusing a feature, isn't the responsibility of the docs to shed some light into misunderstandings?

@lpinca
Copy link
MemberAuthor

lpinca commented Nov 10, 2017

@a0viedo the feature is still documented and there is a dedicated section called "Warning: Using 'uncaughtException' correctly".

This only removes it from the EventEmitter documentation in an attempt to avoid suggesting using 'uncaughtException' as a replacement for missing 'error' listeners.

@Trott
Copy link
Member

Trott commented Nov 13, 2017

Should we link to the "Using uncaughtException correctly" section rather than (or in addition to) recommending a deprecated module? Just a question, not a blocking objection.

@lpinca
Copy link
MemberAuthor

@Trott we can but I would prefer not to. 'error' events should be handled by their respective listener(s) and nothing else. In this context, mentioning 'uncaughtException' in whatever form, is not a good idea imho.

I wanted to also remove the domain recommendation and only keep the last sentence ("As a best practice, listeners should always be added for the 'error' events.") but that seemed too much documentation removed at a single time.

@addaleax
Copy link
Member

Landed in 9531fcb

addaleax pushed a commit that referenced this pull request Nov 18, 2017
Avoid suggesting using `'uncaughtException'` for emitted errors. PR-URL: #16905 Reviewed-By: Ben Noordhuis <[email protected]>
@lpincalpinca deleted the hide/uncaught-exception branch November 18, 2017 20:28
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Avoid suggesting using `'uncaughtException'` for emitted errors. PR-URL: #16905 Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
Avoid suggesting using `'uncaughtException'` for emitted errors. PR-URL: #16905 Reviewed-By: Ben Noordhuis <[email protected]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
Avoid suggesting using `'uncaughtException'` for emitted errors. PR-URL: #16905 Reviewed-By: Ben Noordhuis <[email protected]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.eventsIssues and PRs related to the events subsystem / EventEmitter.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@lpinca@a0viedo@Trott@addaleax@bnoordhuis@MylesBorins@gibfahn@nodejs-github-bot