FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: discourage error event by benjamingr · Pull Request #37264 · nodejs/node · GitHub

/ node Public

doc: discourage error event - #37264

Closed
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:event-target-error-event
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

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 added the doc Issues and PRs related to the documentations. label Feb 7, 2021
benjamingr requested review from Trott and jasnell February 7, 2021 12:27
nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Feb 7, 2021
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 commented Feb 7, 2021

Copy link
Copy Markdown
Contributor

Could you add an entry in deprecations.md?

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 mentioned this pull request Feb 7, 2021
Closed
mcollina requested changes Feb 8, 2021
View reviewed changes
Comment thread doc/api/events.md Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good idea, done

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott reviewed Feb 10, 2021
View reviewed changes
Comment thread doc/api/events.md Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Optional/non-blocking nit:

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

Trott reviewed Feb 10, 2021
View reviewed changes
Comment thread doc/api/events.md Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
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 reviewed Feb 11, 2021
View reviewed changes
Comment thread doc/api/events.md Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We use US spellings, so this:

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

Trott reviewed Feb 11, 2021
View reviewed changes
Comment thread doc/api/events.md Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
before reaching `process.on('uncaughtException')` - this behaviour is
before reaching `process.on('uncaughtException')`. This behavior is

Trott approved these changes Feb 11, 2021
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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 approved these changes Feb 11, 2021
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

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

Copy link
Copy Markdown
Member Author

Landed in cf5f6af

benjamingr closed this Feb 11, 2021
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
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
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 mentioned this pull request May 3, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
Trott Trott approved these changes
addaleax addaleax approved these changes
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

Back | FazBrowse Home | New Git URL