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 — lib: add option to force handling stopped events by atlowChemi · Pull Request #48301 · nodejs/node

Conversation

@atlowChemi

@atlowChemi atlowChemi commented Jun 2, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Refs: whatwg/dom#1152 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Jun 2, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net
  • @nodejs/streams
  • @nodejs/test_runner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@atlowChemi
atlowChemi requested a review from benjamingr June 2, 2023 13:57
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 2, 2023
ljharb
ljharb reviewed Jun 2, 2023
View reviewed changes
Comment thread lib/child_process.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/dgram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/events.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/fs.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/abort_controller.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/readline.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/readline/promises.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/timers/promises.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/timers/promises.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/timers/promises.js Resolved

Uh oh!

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

benjamingr commented Jun 3, 2023

Copy link
Copy Markdown
Member

Good job!

Some comments:

  • It's forcing the run of a listener. In window.onsomething = foo foo is an event handler. In window.addEventListener("something", bar) bar is an event listener. What about kAlwaysRun?
  • I would prefer it if we split adding it to EventTarget from changing all the usages.
  • We need to run the EventTarget benchmark on this to make sure the changes with iterationCondition don't have a negative perf impact.
atlowChemi and MoLow 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.
atlowChemi
atlowChemi commented Jun 3, 2023
View reviewed changes
Comment thread lib/internal/event_target.js Outdated

@atlowChemi atlowChemi Jun 3, 2023

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.

@benjamingr Is this comment still relevant? seems to me like this was handled already

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
MoLow
MoLow reviewed Jun 3, 2023
View reviewed changes
Comment thread lib/internal/event_target.js Resolved

Uh oh!

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

MoLow commented Jun 3, 2023

Copy link
Copy Markdown
Member

+1 on the concerns raised by @benjamingr, I have suggested another name.
also, I would expect to see some tests that break before this fix
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@atlowChemi
atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch 4 times, most recently from 9bbebcc to 8218ad4 Compare June 3, 2023 22:40
@atlowChemi
atlowChemi marked this pull request as ready for review June 3, 2023 22:42
@atlowChemi atlowChemi added eventtarget Issues and PRs related to the EventTarget implementation. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Jun 4, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 4, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 4, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52077/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Jun 5, 2023
CI Reliability 2023-06-05 nodejs/reliability#586
Open
CI Reliability 2023-06-06 nodejs/reliability#587
Open
CI Reliability 2023-06-09 nodejs/reliability#588
Open
@atlowChemi
atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from 8218ad4 to d75ff43 Compare June 13, 2023 13:14
@atlowChemi
atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from d75ff43 to fa4dc42 Compare June 20, 2023 15:00
ljharb
ljharb reviewed Jun 20, 2023
View reviewed changes
Comment thread lib/internal/event_target.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@atlowChemi
lib: add option to force handling stopped events
061ea54
@atlowChemi
atlowChemi force-pushed the avoid-signals-not-working-due-to-propogation-stopped branch from fa4dc42 to 061ea54 Compare June 20, 2023 19:04
benjamingr
benjamingr approved these changes Jun 20, 2023
View reviewed changes
@benjamingr

benjamingr commented Jun 20, 2023

Copy link
Copy Markdown
Member

@ronag @jasnell @mcollina this is a notable behavior change there previously stopImmediatePropagation would prevent core APIs passed signal from being cleaned up.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 20, 2023
@atlowChemi atlowChemi added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jun 20, 2023
@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 21, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 21, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52306/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow approved these changes Jun 21, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Jun 21, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52312/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52338/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52341/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@atlowChemi atlowChemi added the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 22, 2023
@nodejs-github-bot
nodejs-github-bot merged commit bcd35c3 into nodejs:main Jun 22, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2023

Copy link
Copy Markdown
Collaborator

Landed in bcd35c3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@atlowChemi
atlowChemi deleted the avoid-signals-not-working-due-to-propogation-stopped branch June 22, 2023 14:16
This was referenced Jun 23, 2023
CI Reliability 2023-06-23 nodejs/reliability#596
Open
CI Reliability 2023-06-24 nodejs/reliability#597
Open
@atlowChemi atlowChemi mentioned this pull request Jun 25, 2023
lib, net, child_process, streams, dgram, readline, http2: Use addAbortListener where applicable #48550
Closed
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@atlowChemi @RafaelGSS
lib: add option to force handling stopped events
97b00c3 
PR-URL: #48301
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
v20.4.0 proposal #48643
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@atlowChemi @Ceres6
lib: add option to force handling stopped events
cadb8bf 
PR-URL: nodejs#48301
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@atlowChemi @Ceres6
lib: add option to force handling stopped events
dd28606 
PR-URL: nodejs#48301
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@ruyadorno

ruyadorno commented Sep 10, 2023

Copy link
Copy Markdown
Member

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.
atlowChemi reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ruyadorno ruyadorno added the backport-requested-v18.x label Sep 10, 2023
@atlowChemi atlowChemi added the backport-open-v18.x label Sep 10, 2023
atlowChemi added a commit to atlowChemi/node that referenced this pull request Sep 10, 2023
@atlowChemi
lib: add option to force handling stopped events
832ee65 
PR-URL: nodejs#48301
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@atlowChemi

atlowChemi commented Sep 10, 2023

Copy link
Copy Markdown
Member Author

@ruyadorno opened a backport at #49587 🙂
ruyadorno reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
@atlowChemi @ruyadorno
lib: add option to force handling stopped events
dbaa2cc 
PR-URL: #48301
Backport-PR-URL: #49587
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@ruyadorno ruyadorno added backported-to-v18.x and removed backport-requested-v18.x labels Sep 11, 2023
@ruyadorno ruyadorno mentioned this pull request Sep 11, 2023
v18.18.0 proposal #49220
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@atlowChemi @ruyadorno
lib: add option to force handling stopped events
db355d1 
PR-URL: #48301
Backport-PR-URL: #49587
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
@benjamingr benjamingr benjamingr approved these changes
@MoLow MoLow MoLow approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. eventtarget Issues and PRs related to the EventTarget implementation. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

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

@atlowChemi @nodejs-github-bot @benjamingr @MoLow @ruyadorno @ljharb
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.