Skip to content

doc: discourage error event - #37264

Closed
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:event-target-error-event
Closed

doc: discourage error event#37264
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:event-target-error-event

Conversation

@benjamingr

@benjamingr benjamingr commented Feb 7, 2021

Copy link
Copy Markdown
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
@benjamingr
benjamingr requested review from Trott and jasnell February 7, 2021 12:27
@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

aduh95 commented Feb 7, 2021

Copy link
Copy Markdown
Contributor

Could you add an entry in deprecations.md?

@benjamingr

benjamingr commented Feb 7, 2021

Copy link
Copy Markdown
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
Comment thread doc/api/events.md Outdated

@mcollina mcollina Feb 8, 2021

Copy link
Copy Markdown
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.

@benjamingr benjamingr Feb 8, 2021

Copy link
Copy Markdown
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

@benjamingr benjamingr Feb 9, 2021

Copy link
Copy Markdown
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

@benjamingr benjamingr Feb 11, 2021

Copy link
Copy Markdown
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
Comment thread doc/api/events.md Outdated

@Trott Trott Feb 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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
Comment thread doc/api/events.md Outdated

@Trott Trott Feb 10, 2021

Copy link
Copy Markdown
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
Comment thread doc/api/events.md Outdated

@Trott Trott Feb 11, 2021

Copy link
Copy Markdown
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
Comment thread doc/api/events.md Outdated

@Trott Trott Feb 11, 2021

Copy link
Copy Markdown
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

@Trott Trott left a comment

Copy link
Copy Markdown
Member

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

benjamingr commented Feb 11, 2021

Copy link
Copy Markdown
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

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@benjamingr
benjamingr force-pushed the event-target-error-event branch from 322cd88 to 04625f9 Compare February 11, 2021 12:05
@benjamingr
benjamingr force-pushed the event-target-error-event branch from 04625f9 to 36173aa Compare February 11, 2021 12:11
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

benjamingr commented Feb 11, 2021

Copy link
Copy Markdown
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.

6 participants

@benjamingr @aduh95 @mcollina @Trott @addaleax @nodejs-github-bot