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.

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

events: on EventTarget support + once compliance #33659

Closed
wants to merge 5 commits into from
Closed

events: on EventTarget support + once compliance #33659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62e24d9
events: fix EventTarget support
benjamingr May 30, 2020
22d784e
events: support EventTarget in on
benjamingr May 30, 2020
0f2315c
fixup! acorn doesn't like ?.
benjamingr May 30, 2020
a62ff6d
fixup! fix error listener handler case
benjamingr May 30, 2020
d783fc5
fixup! change order for listeners
May 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 49 additions & 35 deletions lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,41 +616,20 @@ 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(
name,
(...args) => { resolve(args); },
{ once: true }
);
return;
}

const eventListener = (...args) => {
if (errorListener !== undefined) {
const errorListener = (err) => {
emitter.removeListener(name, resolver);
reject(err);
};
const resolver = (...args) => {
if (typeof emitter.removeListener === 'function') {
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.
eventTargetAgnosticAddListener(emitter, name, resolver, { once: true });
if (name !== 'error') {
errorListener = (err) => {
emitter.removeListener(name, eventListener);
reject(err);
};

emitter.once('error', errorListener);
addErrorHandlerIfEventEmitter(emitter, errorListener, { once: true });
}

emitter.once(name, eventListener);
});
}

Expand All @@ -661,6 +640,38 @@ function createIterResult(value, done) {
return { value, done };
}

function addErrorHandlerIfEventEmitter(emitter, handler, flags) {
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
if (typeof emitter.on === 'function') {
eventTargetAgnosticAddListener(emitter, 'error', handler, flags);
}
}

function eventTargetAgnosticRemoveListener(emitter, name, listener, flags) {
if (typeof emitter.removeListener === 'function') {
emitter.removeListener(name, listener);
} else if (typeof emitter.removeEventListener === 'function') {
emitter.removeEventListener(name, listener, flags);
} else {
throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter);
}
}

function eventTargetAgnosticAddListener(emitter, name, listener, flags) {
if (typeof emitter.on === 'function') {
if (flags && flags.once) {
emitter.once(name, listener);
} else {
emitter.on(name, listener);
}
} else if (typeof emitter.addEventListener === 'function') {
// EventTarget does not have `error` event semantics like Node
// EventEmitters, we do not listen to `error` events here.
Comment on lines +667 to +668
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment should go to the addErrorHandlerIfEventEmitter.

emitter.addEventListener(name, (arg) => { listener(arg); }, flags);
} else {
throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter);
}
}

function on(emitter, event) {
const unconsumedEvents = [];
const unconsumedPromises = [];
Expand Down Expand Up @@ -697,8 +708,8 @@ function on(emitter, event) {
},

return() {
emitter.removeListener(event, eventHandler);
emitter.removeListener('error', errorHandler);
eventTargetAgnosticRemoveListener(emitter, event, eventHandler);
eventTargetAgnosticRemoveListener(emitter, 'error', errorHandler);
finished = true;

for (const promise of unconsumedPromises) {
Expand All @@ -714,17 +725,20 @@ function on(emitter, event) {
'Error', err);
}
error = err;
emitter.removeListener(event, eventHandler);
emitter.removeListener('error', errorHandler);
eventTargetAgnosticRemoveListener(emitter, event, eventHandler);
eventTargetAgnosticRemoveListener(emitter, 'error', errorHandler);
},

[SymbolAsyncIterator]() {
return this;
}
}, AsyncIteratorPrototype);

emitter.on(event, eventHandler);
emitter.on('error', errorHandler);
eventTargetAgnosticAddListener(emitter, event, eventHandler);
if (event !== 'error') {
addErrorHandlerIfEventEmitter(emitter, errorHandler);
}


return iterator;

Expand Down
49 changes: 48 additions & 1 deletion test/parallel/test-event-on-async-iterator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
const common = require('../common');
const assert = require('assert');
const { on, EventEmitter } = require('events');
const {
EventTarget,
NodeEventTarget,
Event
} = require('internal/event_target');

async function basic() {
const ee = new EventEmitter();
Expand Down Expand Up @@ -204,6 +209,45 @@ async function iterableThrow() {
assert.strictEqual(ee.listenerCount('error'), 0);
}

async function eventTarget() {
const et = new EventTarget();
const tick = () => et.dispatchEvent(new Event('tick'));
const interval = setInterval(tick, 0);
let count = 0;
for await (const [ event ] of on(et, 'tick')) {
count++;
assert.strictEqual(event.type, 'tick');
if (count >= 5) {
break;
}
}
assert.strictEqual(count, 5);
clearInterval(interval);
}

async function errorListenerCount() {
const et = new EventEmitter();
on(et, 'foo');
assert.strictEqual(et.listenerCount('error'), 1);
}

async function nodeEventTarget() {
const et = new NodeEventTarget();
const tick = () => et.dispatchEvent(new Event('tick'));
const interval = setInterval(tick, 0);
let count = 0;
for await (const [ event] of on(et, 'tick')) {
count++;
assert.strictEqual(event.type, 'tick');
if (count >= 5) {
break;
}
}
assert.strictEqual(count, 5);
clearInterval(interval);
}

lundibundi marked this conversation as resolved.
Show resolved Hide resolved

async function run() {
const funcs = [
basic,
Expand All @@ -212,7 +256,10 @@ async function run() {
throwInLoop,
next,
nextError,
iterableThrow
iterableThrow,
eventTarget,
errorListenerCount,
nodeEventTarget
];

for (const fn of funcs) {
Expand Down
98 changes: 23 additions & 75 deletions test/parallel/test-events-once.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,55 +1,10 @@
'use strict';
// Flags: --expose-internals

const common = require('../common');
const { once, EventEmitter } = require('events');
const { strictEqual, deepStrictEqual } = require('assert');

class EventTargetMock {
constructor() {
this.events = {};
}

addEventListener = common.mustCall(function(name, listener, options) {
if (!(name in this.events)) {
this.events[name] = { listeners: [], options };
}
this.events[name].listeners.push(listener);
});

removeEventListener = common.mustCall(function(name, callback) {
if (!(name in this.events)) {
return;
}
const event = this.events[name];
const stack = event.listeners;

for (let i = 0, l = stack.length; i < l; i++) {
if (stack[i] === callback) {
stack.splice(i, 1);
if (stack.length === 0) {
Reflect.deleteProperty(this.events, name);
}
return;
}
}
});

dispatchEvent = function(name, ...arg) {
if (!(name in this.events)) {
return true;
}
const event = this.events[name];
const stack = event.listeners.slice();

for (let i = 0, l = stack.length; i < l; i++) {
stack[i].apply(this, arg);
if (event.options.once) {
this.removeEventListener(name, stack[i]);
}
}
return !name.defaultPrevented;
};
}
const { strictEqual, deepStrictEqual, fail } = require('assert');
const { EventTarget, Event } = require('internal/event_target');

async function onceAnEvent() {
const ee = new EventEmitter();
Expand Down Expand Up @@ -104,8 +59,6 @@ async function stopListeningAfterCatchingError() {
ee.emit('myevent', 42, 24);
});

process.on('multipleResolves', common.mustNotCall());

try {
await once(ee, 'myevent');
} catch (_e) {
Expand All @@ -125,54 +78,49 @@ async function onceError() {
ee.emit('error', expected);
});

const [err] = await once(ee, 'error');
const promise = once(ee, 'error');
strictEqual(ee.listenerCount('error'), 1);
const [ err ] = await promise;
strictEqual(err, expected);
strictEqual(ee.listenerCount('error'), 0);
strictEqual(ee.listenerCount('myevent'), 0);
}

async function onceWithEventTarget() {
const et = new EventTargetMock();

const et = new EventTarget();
const event = new Event('myevent');
process.nextTick(() => {
et.dispatchEvent('myevent', 42);
et.dispatchEvent(event);
});
const [ value ] = await once(et, 'myevent');
strictEqual(value, 42);
strictEqual(Reflect.has(et.events, 'myevent'), false);
}

async function onceWithEventTargetTwoArgs() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @JeniaBR

I think Domenic was right on #29498 (comment) .

This PR still uses the same array wrapping (for compatibility with EventEmitter but no longer supports multiple arguments in event dispatch (as that has always been impossible anyway).

const et = new EventTargetMock();

process.nextTick(() => {
et.dispatchEvent('myevent', 42, 24);
});

const value = await once(et, 'myevent');
deepStrictEqual(value, [42, 24]);
strictEqual(value, event);
}

async function onceWithEventTargetError() {
const et = new EventTargetMock();

const expected = new Error('kaboom');
const et = new EventTarget();
const error = new Event('error');
process.nextTick(() => {
et.dispatchEvent('error', expected);
et.dispatchEvent(error);
});

const [err] = await once(et, 'error');
strictEqual(err, expected);
strictEqual(Reflect.has(et.events, 'error'), false);
const [ err ] = await once(et, 'error');
strictEqual(err, error);
}

async function prioritizesEventEmitter() {
const ee = new EventEmitter();
ee.addEventListener = fail;
ee.removeAllListeners = fail;
process.nextTick(() => ee.emit('foo'));
await once(ee, 'foo');
}
lundibundi marked this conversation as resolved.
Show resolved Hide resolved
Promise.all([
onceAnEvent(),
onceAnEventWithTwoArgs(),
catchesErrors(),
stopListeningAfterCatchingError(),
onceError(),
onceWithEventTarget(),
onceWithEventTargetTwoArgs(),
onceWithEventTargetError(),
prioritizesEventEmitter(),
]).then(common.mustCall());