-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: support EventTarget in once
#29498
Changes from all commits
c48786e
0b69f8b
1a5f42d
ddd3efa
9504593
3526b46
6f13b52
646ae69
b8d9389
510270a
126494c
0cf3e5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -497,6 +497,17 @@ function unwrapListeners(arr) { | |
|
||
function once(emitter, name) { | ||
return new Promise((resolve, reject) => { | ||
if (typeof emitter.addEventListener === 'function') { | ||
// EventTarget does not have `error` event semantics like Node | ||
// EventEmitters, we do not listen to `error` events here. | ||
emitter.addEventListener( | ||
mcollina marked this conversation as resolved.
Show resolved
Hide resolved
|
||
name, | ||
(...args) => { resolve(args); }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EventTarget is guaranteed to only have one argument, so you could avoid passing an array here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @domenic what do you think is the right call API wise in terms of compatibility with EventEmitter and the coming (?) events proposal? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't pay much attention to early-stage TC39 proposals. But I think just compatibility with how events work in event listeners in the DOM would suggest using a single argument instead of an array containing that argument. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this now resolves with an array, and just got released in Node v12.11.0. Are we now stuck with this promise signature? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think changing that would be a breaking change, yes. That being said, I think there is some value in keeping this consistent between EventTarget and EventEmitter, so I’m not really sure that we should change this even if we could. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about consistency with web EventTarget? If that is a goal, arrays should not get involved. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@domenic Yes, there’s a tradeoff, but at least to me a polymorphic API seems worse here than one that performs some unnecessary boxing. That being said: I personally don’t like the decision to use arrays in the first place, but I think we’re stuck with that now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW I'm looking at this and I'm pretty sure Domenic was right and our implementation is wrong. Even for consistency. Our tests incorrectly use an EventTargetMock that accepts multiple arguments (which isn't possible) and dispatches strings (and not objects). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in #33659 |
||
{ once: true } | ||
); | ||
return; | ||
} | ||
|
||
const eventListener = (...args) => { | ||
if (errorListener !== undefined) { | ||
emitter.removeListener('error', errorListener); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something, an example showing the use of the EventTarget API would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasnell something like that?
Not sure about this line
const EventTarget = require('../event-target-implementation')
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can defer doing that to a future pull request. Mostly this is for web compatibility and for making EE more universal JavaScript compatible but our docs on the other hand mostly ignore that as far as I know.