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.

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

events: allow an event to be dispatched multiple times #39395

Closed
wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jul 15, 2021

Use a different flag to prevent recursive dispatching.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 15, 2021
Use a different flag to prevent recursive dispatching.
@lpinca lpinca changed the title events: reset the event target to null events: allow an event to be dispatched multiple times Jul 21, 2021
@lpinca
Copy link
Member Author

lpinca commented Jul 21, 2021

@aduh95 PTAL.

@@ -151,12 +153,12 @@ class Event {
// These are not supported in Node.js and are provided purely for
// API completeness.

composedPath() { return this[kTarget] ? [this[kTarget]] : []; }
composedPath() { return this[kIsBeingDispatched] ? [this[kTarget]] : []; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR: Chromium and Safari both return an empty array when dispatching an event. Firefox has the same behavior as Node.js. I couldn't find which behavior is spec compliant.

{
  // Same event dispatched multiple times.
  const event = new Event('foo');
  const eventTarget1 = new EventTarget();
  const eventTarget2 = new EventTarget();

  eventTarget1.addEventListener('foo', ((event) => {
    console.log(event.target===eventTarget1, event.eventPhase===Event.AT_TARGET); // true true
    const path = event.composedPath();
    console.log(path.length === 1, path[0]===eventTarget1); // depends on the browser:
    // On Firefox + Node.js: true true
    // On Safari + Chromium : false false
  }));

  eventTarget2.addEventListener('foo', ((event) => {
    console.log(event.target===eventTarget2, event.eventPhase===Event.AT_TARGET); // true true
    const path = event.composedPath();
    console.log(path.length === 1, path[0]===eventTarget2); // depends on the browser
  }));

  eventTarget1.dispatchEvent(event);
  console.log(event.target===eventTarget1, event.eventPhase===Event.NONE); // true true
  console.log(event.composedPath().length === 0); // true

  eventTarget2.dispatchEvent(event);
  console.log(event.target===eventTarget2, event.eventPhase===Event.NONE); // true true
  console.log(event.composedPath().length === 0); // true
}

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 21, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the eventtarget Issues and PRs related to the EventTarget implementation. label Jul 25, 2021
lpinca added a commit that referenced this pull request Jul 25, 2021
Use a different flag to prevent recursive dispatching.

PR-URL: #39395
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca
Copy link
Member Author

lpinca commented Jul 25, 2021

Landed in 5c4e673.

@lpinca lpinca closed this Jul 25, 2021
@lpinca lpinca deleted the fix/target-reset branch July 25, 2021 14:18
targos pushed a commit that referenced this pull request Jul 26, 2021
Use a different flag to prevent recursive dispatching.

PR-URL: #39395
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Jul 26, 2021
richardlau pushed a commit that referenced this pull request Jul 29, 2021
Use a different flag to prevent recursive dispatching.

PR-URL: #39395
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Jul 29, 2021
Use a different flag to prevent recursive dispatching.

PR-URL: #39395
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
mrbbot added a commit to mrbbot/node that referenced this pull request Aug 15, 2021
Fast path for EventTarget dispatch with no listeners didn't reset
kIsBeingDispatched flag, meaning same event couldn't be dispatched
multiple times.

Refs: nodejs#39395
mrbbot added a commit to mrbbot/node that referenced this pull request Aug 15, 2021
Fast path for EventTarget dispatch with no listeners didn't reset
kIsBeingDispatched flag, meaning same event couldn't be dispatched
multiple times.

Refs: nodejs#39395
lpinca pushed a commit that referenced this pull request Sep 26, 2021
Fast path for EventTarget dispatch with no listeners didn't reset
kIsBeingDispatched flag, meaning same event couldn't be dispatched
multiple times.

PR-URL: #39772
Refs: #39395
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Oct 4, 2021
Fast path for EventTarget dispatch with no listeners didn't reset
kIsBeingDispatched flag, meaning same event couldn't be dispatched
multiple times.

PR-URL: #39772
Refs: #39395
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. eventtarget Issues and PRs related to the EventTarget implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants