Skip to content

events: deal with Symbol() passed to event constructor #33612

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

events: deal with Symbol() passed to event constructor #33612

wants to merge 2 commits into from

Conversation

@benjamingr
Copy link
Member

@benjamingr benjamingr commented May 28, 2020

Align behavior of Event constructor when a symbol is passed with what the DOM does.

Old behavior:

  • new Event(Symbol()) creates an event with type "Symbol()"
    New behavior:
  • new Event(Symbol()) throws an error (like browsers do).

cc @jasnell

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

@benjamingr benjamingr requested a review from jasnell May 28, 2020 14:29
@benjamingr benjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label May 28, 2020
targos
targos reviewed May 28, 2020
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 28, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31610/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 28, 2020
@benjamingr benjamingr mentioned this pull request May 29, 2020
Closed
11 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 29, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31637/

@jasnell jasnell added the fast-track PRs that do not need to wait for 48 hours to land. label May 29, 2020
@jasnell
Copy link
Member

jasnell commented May 29, 2020

Fixups to unrelease internal feature. Fast track?

@BridgeAR
Copy link
Member

BridgeAR commented May 30, 2020

@benjamingr this needs a rebase.

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2020
@benjamingr
Copy link
Member Author

benjamingr commented May 30, 2020

@BridgeAR thanks, rebased :]

@benjamingr benjamingr force-pushed the event-constructor-symbol-passed branch from 9031d99 to 1cd9500 Compare May 30, 2020 11:29
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 30, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31656/

@BridgeAR BridgeAR 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 benjamingr force-pushed the event-constructor-symbol-passed branch 2 times, most recently from bbb1b12 to 3749047 Compare May 31, 2020 11:44
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@benjamingr benjamingr force-pushed the event-constructor-symbol-passed branch from e9f1ce2 to 0a2edb5 Compare May 31, 2020 12:59
@BridgeAR BridgeAR removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels May 31, 2020
@BridgeAR
fixup!
56f3b58 
Signed-off-by: Ruben Bridgewater <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 3, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31704/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 3, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31706/

jasnell pushed a commit that referenced this pull request Jun 3, 2020
@jasnell
events: deal with Symbol() passed to event constructor
3a7a5d7 
PR-URL: #33612
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jun 3, 2020

Landed in 3a7a5d7

@jasnell jasnell closed this Jun 3, 2020
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@codebytere
events: deal with Symbol() passed to event constructor
e7bce2e 
PR-URL: #33612
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@codebytere
events: deal with Symbol() passed to event constructor
440ab41 
PR-URL: #33612
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax addaleax mentioned this pull request Sep 22, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@juanarbol juanarbol juanarbol approved these changes

Assignees

No one assigned

Labels

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.

7 participants

@benjamingr @nodejs-github-bot @jasnell @BridgeAR @addaleax @targos @juanarbol