GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: discourage error event by benjamingr · Pull Request #37264 · nodejs/node

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95, ExE-Boss, and RaisinTen reacted with thumbs up emoji Linkgoron reacted with rocket emoji
All reactions
  • 👍 3 reactions
  • 🚀 1 reaction
@benjamingr benjamingr added the doc Issues and PRs related to Node.js documentation. 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 EventEmitter and the events module. 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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
aduh95 and ExE-Boss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr mentioned this pull request Feb 7, 2021
events: change EventTarget handler exception behavior #37237
Closed
addaleax
addaleax approved these changes Feb 7, 2021
View reviewed changes
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

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@benjamingr benjamingr Feb 8, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good idea, done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@benjamingr benjamingr Feb 9, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@benjamingr benjamingr Feb 11, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Trott
Trott reviewed Feb 10, 2021
View reviewed changes
Comment thread doc/api/events.md Outdated

@Trott Trott Feb 10, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Trott
Trott approved these changes Feb 11, 2021
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Feb 11, 2021
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@benjamingr
benjamingr force-pushed the event-target-error-event branch from 322cd88 to 04625f9 Compare February 11, 2021 12:05
@benjamingr
doc: discourage error event
36173aa
@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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@benjamingr

benjamingr commented Feb 11, 2021

Copy link
Copy Markdown
Member Author

Landed in cf5f6af
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This was referenced Feb 16, 2021
v15.9.0 proposal #37405
Closed
v15.9.0 proposal #37406
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. events Issues and PRs related to EventEmitter and the events module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@benjamingr @aduh95 @mcollina @Trott @addaleax @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.