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.

Sign up for GitHub

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

Jump to bottom

doc: discourage error event #37264

Closed
wants to merge 1 commit into from
Closed

doc: discourage error event #37264

wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

Refs: #37237 (comment)

Refs: #37237

Basically - the "error" event is being removed from the code - it was never documented on process but in order to be cautious the change is still semver-major.

This PR changes the wording to indicate the future behaviour.

Bikeshedding/suggestions welcome

@benjamingr benjamingr added the doc Issues and PRs related to the documentations. label Feb 7, 2021
@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Feb 7, 2021
@aduh95 aduh95 added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 7, 2021
@aduh95
Copy link
Contributor

aduh95 commented Feb 7, 2021

Could you add an entry in deprecations.md?

@benjamingr
Copy link
Member Author

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

@benjamingr benjamingr mentioned this pull request Feb 7, 2021
Closed
mcollina
mcollina requested changes Feb 8, 2021
View reviewed changes
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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcollina any chance this can get a second review? I'd prefer to make sure the changes are what you had in mind

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcollina ping (I'll also update it with Trott's suggestions before landing)

Trott
Trott reviewed Feb 10, 2021
View reviewed changes
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.
Copy link
Member

@Trott Trott Feb 10, 2021 •
edited

Choose a reason for hiding this comment

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

Optional/non-blocking nit:

Suggested change
crash the Node.js process by default.
terminate the Node.js process by default.

Trott
Trott reviewed Feb 10, 2021
View reviewed changes
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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type events like `EventEmitter` does in order to be spec compliant.
type events like `EventEmitter` does to be spec compliant.

...or even:

Suggested change
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.

Trott
Trott reviewed Feb 11, 2021
View reviewed changes
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.
Copy link
Member

Choose a reason for hiding this comment

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

We use US spellings, so this:

Suggested change
be aligned with the new behaviour.
be aligned with the new behavior.

Trott
Trott reviewed Feb 11, 2021
View reviewed changes
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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
before reaching `process.on('uncaughtException')` - this behaviour is
before reaching `process.on('uncaughtException')`. This behavior is

Trott
Trott approved these changes Feb 11, 2021
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with US spelling applied to behaviour/behavior. Other changes are optional/non-blocking.

@benjamingr
Copy link
Member Author

@Trott any chance you could also review #37237 ? Landing this one doesn't make a ton of sense without that one :)

mcollina
mcollina approved these changes Feb 11, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

benjamingr added a commit that referenced this pull request Feb 11, 2021
@benjamingr
doc: discourage error event
cf5f6af 
PR-URL: #37264
Refs: #37237
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@benjamingr
Copy link
Member Author

Landed in cf5f6af

@benjamingr benjamingr closed this Feb 11, 2021
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@benjamingr @danielleadams
doc: discourage error event
3464c9f 
PR-URL: #37264
Refs: #37237
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Feb 16, 2021
Closed
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@benjamingr @targos
doc: discourage error event
b7c9366 
PR-URL: #37264
Refs: #37237
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@benjamingr @aduh95 @mcollina @Trott @addaleax @nodejs-github-bot