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: deal with no argument case #33611

Closed
wants to merge 5 commits into from

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-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label May 28, 2020
@benjamingr benjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label May 28, 2020
@benjamingr benjamingr force-pushed the event-controller-compat branch from 48b30da to cea4961 Compare May 28, 2020 14:19
@benjamingr
Copy link
Member Author

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR 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 <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@benjamingr
Copy link
Member Author

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 :]

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented Jun 3, 2020

@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
Member Author

@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 <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jasnell
Copy link
Member

jasnell commented Jun 5, 2020

Landed in 2362378

@jasnell jasnell closed this Jun 5, 2020
codebytere pushed a commit that referenced this pull request Jun 18, 2020
PR-URL: #33611
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
PR-URL: #33611
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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. errors Issues and PRs related to JavaScript errors originated in Node.js core. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants