Skip to content

Conversation

@benjamingr
Copy link
Member

Fix new Event() to throw an error rather than behave like new Event(undefined) to align with browser behavior.

  • 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

cc @jasnell

I'll be making a few of these (compatibility) PRs to align with Chrome's behavior as I run into issues and eventually port the WPTs (as suggested by @targos).

I'm keeping these small so it's easier to bikeshed things like error codes.

@nodejs-github-botnodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label May 28, 2020
@benjamingrbenjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label May 28, 2020
@benjamingrbenjamingrforce-pushed the event-controller-compat branch from 48b30da to cea4961CompareMay 28, 2020 14:19
@benjamingr
Copy link
MemberAuthor

@targos is this more of what you had in mind?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 29, 2020
benjamingr added a commit that referenced this pull request May 31, 2020
PR-URL: #33611 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@benjamingr
Copy link
MemberAuthor

benjamingr commented May 31, 2020

Landed in 8ae28ff

@BridgeAR thanks for the help landing things with ncu - please take a look and make sure I didn't dum-goofed :]

@lundibundilundibundi mentioned this pull request May 31, 2020
2 tasks
@BridgeARBridgeAR reopened this May 31, 2020
@benjamingrbenjamingrforce-pushed the event-controller-compat branch from c4961e4 to 7a1f27eCompareMay 31, 2020 12:53
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

@benjamingr ... the CI failure here can be fixed with the following change:

diff --git a/test/parallel/test-eventtarget.js b/test/parallel/test-eventtarget.js index 82a89caae1..783ca5eeab 100644 --- a/test/parallel/test-eventtarget.js+++ b/test/parallel/test-eventtarget.js@@ -408,6 +408,6 @@ ok(EventTarget);{const target = new EventTarget(); strictEqual(target.toString(), '[object EventTarget]'); - const event = new Event();+ const event = new Event(''); strictEqual(event.toString(), '[object Event]')}

@benjamingr
Copy link
MemberAuthor

@jasnell pushed a fix, feel free to push such fixed on my (ET) branches in the future and thanks for landing.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

jasnell pushed a commit that referenced this pull request Jun 5, 2020
PR-URL: #33611 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@jasnell
Copy link
Member

Landed in 2362378

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.errorsIssues and PRs related to JavaScript errors originated in Node.js core.eventsIssues and PRs related to the events subsystem / EventEmitter.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

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