Skip to content

events: support dispatching event from event - #33624

Closed
benjamingr wants to merge 5 commits into
nodejs:masterfrom
benjamingr:event-target-fix-recursion
Closed

events: support dispatching event from event#33624
benjamingr wants to merge 5 commits into
nodejs:masterfrom
benjamingr:event-target-fix-recursion

Conversation

@benjamingr

Copy link
Copy Markdown
Member

Currently our events guard against dispatching an event from itself by type rather than by event instance. This makes a WPT test fail.

Here's some code to demonstrate the issue.

{consttarget=newEventTarget();constevent=newEvent('foo');letcallCount=0;target.addEventListener('foo',(()=>{callCount++;if(callCount<5){target.dispatchEvent(newEvent('foo'));}},5));console.log(callCount)}

Dispatching an event while handling it is still not supported (as required).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@benjamingrbenjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label May 28, 2020
@benjamingr
benjamingr requested a review from jasnellMay 28, 2020 19:04
@benjamingrbenjamingr mentioned this pull request May 28, 2020
4 tasks
@benjamingr

Copy link
Copy Markdown
MemberAuthor

@jasnell I think you might have missed this one :]

Comment threadlib/internal/event_target.js Outdated
Comment threadlib/internal/event_target.js Outdated
@benjamingr
benjamingrforce-pushed the event-target-fix-recursion branch from 950969e to 3b6e3f1CompareMay 30, 2020 11:34
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2020
@benjamingr
benjamingrforce-pushed the event-target-fix-recursion branch from 3b6e3f1 to adaed1bCompareMay 31, 2020 11:50
@BridgeAR
BridgeARforce-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72CompareMay 31, 2020 12:18
@BridgeAR
BridgeARforce-pushed the event-target-fix-recursion branch from adaed1b to f24f1b5CompareMay 31, 2020 13:38
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@jasnell

Copy link
Copy Markdown
Member

Ping @benjamingr ... tests are failing on this one.

@jasnelljasnell added eventtarget Issues and PRs related to the EventTarget implementation. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 19, 2020
@benjamingr

Copy link
Copy Markdown
MemberAuthor

@jasnell sorry, I was (mostly) AFK and am (mostly) AFK for the coming week (except the summit). I will do one pass over all the PRs you pinged me at and fix/rebase them over master.

I feel like this landing part is harder than it should so feel free to either squash them all into one PR and land it as one change, or to just make all the changes directly (I really don't mind the git attribution being precise) or whatever is most convenient to you.

Again: my objective is to be helpful and to do the least toe-stepping :]

Event#cancelBubble is property (and not a function). Change
Event#cancelBubble to a property and add a test.
PR-URL: nodejs#33613
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
PR-URL: nodejs#33623
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@benjamingr
benjamingrforce-pushed the event-target-fix-recursion branch from 92d841d to 5b397abCompareJune 21, 2020 10:17
@jasnell

Copy link
Copy Markdown
Member

@benjamingr ... yeah, it's been pretty painful doing these piece by piece. I think I'll go ahead and combine all the changes into a single PR.

@benjamingr

Copy link
Copy Markdown
MemberAuthor

@jasnell I am starting to see what you meant by "take it to a vendored dep" back then :]

Anyway, whatever is easiest and least painful for you. Another thing we can do is open one PR that will stay open for a few days on a branch, then we can push all the changes to that branch and merge that after a few days or something.

@jasnelljasnell mentioned this pull request Jun 22, 2020
3 tasks
@jasnell

Copy link
Copy Markdown
Member

Closing in favor of #34015 (which combines this and other eventtarget PRs into a single to make landing easier

@jasnelljasnell closed this Jun 22, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

eventsIssues and PRs related to the events subsystem / EventEmitter.eventtargetIssues and PRs related to the EventTarget implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@benjamingr@nodejs-github-bot@jasnell@BridgeAR