-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
#27977
Changes from all commits
de50efd
8cd36cd
cb5fecd
99117f8
774f06e
fc09d9a
6fc65f0
c409d35
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 |
---|---|---|
|
@@ -494,21 +494,31 @@ function once(emitter, name) { | |
}; | ||
let errorListener; | ||
|
||
// Adding an error listener is not optional because | ||
// if an error is thrown on an event emitter we cannot | ||
// guarantee that the actual event we are waiting will | ||
// be fired. The result could be a silent way to create | ||
// memory or file descriptor leaks, which is something | ||
// we should avoid. | ||
if (name !== 'error') { | ||
errorListener = (err) => { | ||
emitter.removeListener(name, eventListener); | ||
reject(err); | ||
}; | ||
|
||
emitter.once('error', errorListener); | ||
} | ||
|
||
emitter.once(name, eventListener); | ||
if (typeof emitter.once === 'function' && | ||
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 could be going totally crazy but I feel like this function is incorrect and the test only passes because it runs through the EventEmitter path? I think you need something more like this? function once(emitter, name) {
return new Promise((resolve, reject) => {
if (typeof emitter.once === 'function' &&
typeof emitter.removeListener === 'function') {
const eventListener = (...args) => {
if (errorListener !== undefined) {
emitter.removeListener('error', errorListener);
}
resolve(args);
};
let errorListener;
// Adding an error listener is not optional because
// if an error is thrown on an event emitter we cannot
// guarantee that the actual event we are waiting will
// be fired. The result could be a silent way to create
// memory or file descriptor leaks, which is something
// we should avoid.
if (name !== 'error') {
errorListener = (err) => {
emitter.removeListener(name, eventListener);
reject(err);
};
emitter.once('error', errorListener);
}
emitter.once(name, eventListener);
return;
}
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(name, resolve, { once: true });
return;
}
throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter);
});
} 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 am not sure what the difference here - but this can be a brainfart on my behalf 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. Your original PR still binds the 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. @apapirovski ah wow you're right! |
||
typeof emitter.removeListener === 'function') { | ||
// Adding an error listener is not optional because | ||
// if an error is thrown on an event emitter we cannot | ||
// guarantee that the actual event we are waiting will | ||
// be fired. The result could be a silent way to create | ||
// memory or file descriptor leaks, which is something | ||
// we should avoid. | ||
if (name !== 'error') { | ||
errorListener = (err) => { | ||
emitter.removeListener(name, eventListener); | ||
reject(err); | ||
}; | ||
emitter.once('error', errorListener); | ||
} | ||
return emitter.once(name, eventListener); | ||
} | ||
if (typeof emitter.addEventListener === 'function') { | ||
return new Promise((resolve) => { | ||
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. Extra Promise not needed here? |
||
// Although EventTarget does not have `error` event semantics like Node | ||
// EventEmitters, we do not listen to `error` events here. | ||
emitter.addEventListener(name, resolve, { once: true }); | ||
}); | ||
} | ||
throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,10 +84,31 @@ async function onceError() { | |
strictEqual(ee.listenerCount('myevent'), 0); | ||
} | ||
|
||
async function onceWithEventTarget() { | ||
const emitter = new class EventTargetLike extends EventEmitter { | ||
addEventListener = common.mustCall(function (name, listener, options) { | ||
if (options.once) { | ||
this.once(name, listener); | ||
} else { | ||
this.on(name, listener); | ||
} | ||
}); | ||
}; | ||
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. Add |
||
// make sure we don't bind as EventEmitter | ||
emitter.addListener = emitter.on = undefined; | ||
process.nextTick(() => { | ||
emitter.emit('myevent', 42); | ||
}); | ||
const [value] = await once(emitter, 'myevent'); | ||
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. Also check that there are no interesting error semantics? |
||
strictEqual(value, 42); | ||
strictEqual(emitter.listenerCount('myevent'), 0); | ||
} | ||
|
||
Promise.all([ | ||
onceAnEvent(), | ||
onceAnEventWithTwoArgs(), | ||
catchesErrors(), | ||
stopListeningAfterCatchingError(), | ||
onceError() | ||
onceError(), | ||
onceWithEventTarget() | ||
]).then(common.mustCall()); |
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.
Worth mentioning the different argument semantics? It resolves with the
Event
instance rather than an array of arguments.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.
@apapirovski I was not sure about what to do here - what behavior would you think is less surprising?
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.
More saying we should make clear the distinction in arguments that the callback receives when you use this with
EventTarget
.