Skip to content
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

doc: avoid mentioning 'uncaughtException' #16905

Closed
wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca 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-bot nodejs-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

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

a0viedo commented Nov 10, 2017

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
Member Author

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
Member Author

lpinca commented Nov 14, 2017

@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 addaleax closed this Nov 18, 2017
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]>
@lpinca lpinca 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]>
@MylesBorins MylesBorins 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]>
@gibfahn gibfahn 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]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants