doc: discourage error event#37264
doc: discourage error event #37264
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the events 
aduh95 added semver-major 
aduh95 commented Feb 7, 2021
Could you add an entry in |
benjamingr commented Feb 7, 2021
@aduh95 not technically deprecated since not technically documented 😅 I am happy to add it to deprecations.md though this isn't going through the regular cycle. |
doc/api/events.md Outdated
| Currently errors are first forwarded to the `process.on('error')` event | ||
| before reaching `process.on('uncaughtException')` - this behaviour will change | ||
| in the next major release to align `EventTarget` with other Node.js APIs. Any |
There was a problem hiding this comment.
I don't like naming "the next major release". I prefer if we say it is deprecated and will be removed in a future major release.
There was a problem hiding this comment.
@mcollina any chance this can get a second review? I'd prefer to make sure the changes are what you had in mind
There was a problem hiding this comment.
@mcollina ping (I'll also update it with Trott's suggestions before landing)
doc/api/events.md Outdated
| the other registered handlers from being invoked. | ||
| by default the error is treated as an uncaught exception on | ||
| `process.nextTick()`. This means uncaught exceptions in `EventTarget`s will | ||
| crash the Node.js process by default. |
There was a problem hiding this comment.
Optional/non-blocking nit:
| crash the Node.js process by default. | |
| terminate the Node.js process by default. |
doc/api/events.md Outdated
| from being invoked. | ||
| The `EventTarget` does not implement any special default handling for `'error'` | ||
| type events like `EventEmitter` does in order to be spec compliant. |
There was a problem hiding this comment.
| type events like `EventEmitter` does in order to be spec compliant. | |
| type events like `EventEmitter` does to be spec compliant. |
...or even:
| type events like `EventEmitter` does in order to be spec compliant. | |
| type events like `EventEmitter` does. |
It's not clear without additional context which is being spec compliant. Is it EventTarget being spec compliant but not implementing it? Or is it EventEmitter being spec compliant by implementing it? (I know the answer, or at least I think I do, but a reader may not.) Then again, maybe it's not even relevant, hence the removal suggestion above.
doc/api/events.md Outdated
| before reaching `process.on('uncaughtException')` - this behaviour is | ||
| deprecated and will change in a future release to align `EventTarget` with | ||
| other Node.js APIs. Any code relying on the `process.on('error')` event should | ||
| be aligned with the new behaviour. |
There was a problem hiding this comment.
We use US spellings, so this:
| be aligned with the new behaviour. | |
| be aligned with the new behavior. |
doc/api/events.md Outdated
| type events like `EventEmitter` does in order to be spec compliant. | ||
| Currently errors are first forwarded to the `process.on('error')` event | ||
| before reaching `process.on('uncaughtException')` - this behaviour is |
There was a problem hiding this comment.
| before reaching `process.on('uncaughtException')` - this behaviour is | |
| before reaching `process.on('uncaughtException')`. This behavior is |
Trott left a comment
Trott left a comment There was a problem hiding this comment.
LGTM with US spelling applied to behaviour/behavior. Other changes are optional/non-blocking.
benjamingr commented Feb 11, 2021
benjamingrforce-pushed the event-target-error-event branch from 322cd88 to 04625f9Comparebenjamingrforce-pushed the event-target-error-event branch from 04625f9 to 36173aaComparePR-URL: #37264 Refs: #37237 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>
benjamingr commented Feb 11, 2021
Landed in cf5f6af |
PR-URL: #37264 Refs: #37237 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: #37264 Refs: #37237 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>
6 participants
Refs: #37237 (comment)
Refs: #37237
Basically - the "error" event is being removed from the code - it was never documented on
processbut in order to be cautious the change is still semver-major.This PR changes the wording to indicate the future behaviour.
Bikeshedding/suggestions welcome