From c48786ea3f870c3ba3ab71655c919127898cf3d0 Mon Sep 17 00:00:00 2001 From: Jenia Date: Mon, 19 Aug 2019 01:13:26 +0300 Subject: [PATCH 01/12] events: add support for EventTarget in once --- lib/events.js | 52 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/lib/events.js b/lib/events.js index ebde85c6ddf990..620eab26bad06c 100644 --- a/lib/events.js +++ b/lib/events.js @@ -497,29 +497,41 @@ function unwrapListeners(arr) { function once(emitter, name) { return new Promise((resolve, reject) => { - 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); + if (emitter.addEventListener === undefined) { + 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; + } - emitter.once('error', errorListener); + 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; } - emitter.once(name, eventListener); + throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter); }); } From 0b69f8b9c5be4181c3819904d03d15729fa76b1e Mon Sep 17 00:00:00 2001 From: Jenia Date: Mon, 19 Aug 2019 01:14:15 +0300 Subject: [PATCH 02/12] events: add test onceWithEventTarget --- test/parallel/test-events-once.js | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-events-once.js b/test/parallel/test-events-once.js index 25ef4e9845422c..2e95e581eb8816 100644 --- a/test/parallel/test-events-once.js +++ b/test/parallel/test-events-once.js @@ -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); + } + }); + }(); + + process.nextTick(() => { + emitter.emit('myevent', 42); + }); + const value = await once(emitter, 'myevent'); + strictEqual(value, 42); + strictEqual(emitter.listenerCount('myevent'), 0); + +} + Promise.all([ onceAnEvent(), onceAnEventWithTwoArgs(), catchesErrors(), stopListeningAfterCatchingError(), - onceError() + onceError(), + onceWithEventTarget() ]).then(common.mustCall()); From 1a5f42d589a1fb6b7c29fdebd237c70b496caf98 Mon Sep 17 00:00:00 2001 From: Jenia Date: Tue, 3 Sep 2019 22:51:01 +0300 Subject: [PATCH 03/12] events: add handling for error --- lib/events.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/events.js b/lib/events.js index 620eab26bad06c..678db2bfa98d75 100644 --- a/lib/events.js +++ b/lib/events.js @@ -497,7 +497,7 @@ function unwrapListeners(arr) { function once(emitter, name) { return new Promise((resolve, reject) => { - if (emitter.addEventListener === undefined) { + if (typeof emitter.addEventListener === 'undefined') { const eventListener = (...args) => { if (errorListener !== undefined) { emitter.removeListener('error', errorListener); @@ -528,7 +528,15 @@ function once(emitter, name) { 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 }); + if (name === 'error') { + reject('EventTarget does not have `error` event semantics'); + } + + emitter.addEventListener( + name, + (...args) => { resolve(args); }, + { once: true } + ); return; } From ddd3efab54f2cfbc1bc76dd8a056a6d42a64eb99 Mon Sep 17 00:00:00 2001 From: Jenia Date: Tue, 3 Sep 2019 22:51:12 +0300 Subject: [PATCH 04/12] events: add more tests --- test/parallel/test-events-once.js | 38 +++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-events-once.js b/test/parallel/test-events-once.js index 2e95e581eb8816..abfcfab8518945 100644 --- a/test/parallel/test-events-once.js +++ b/test/parallel/test-events-once.js @@ -98,10 +98,42 @@ async function onceWithEventTarget() { process.nextTick(() => { emitter.emit('myevent', 42); }); - const value = await once(emitter, 'myevent'); + const [ value ] = await once(emitter, 'myevent'); strictEqual(value, 42); strictEqual(emitter.listenerCount('myevent'), 0); +} +async function onceWithEventTargetTwoArgs() { + 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); + } + }); + }(); + + process.nextTick(() => { + emitter.emit('myevent', 42, 24); + }); + + const value = await once(emitter, 'myevent'); + deepStrictEqual(value, [42, 24]); +} + +async function onceWithEventTargetError() { + const emitter = new EventEmitter(); + emitter.addEventListener = () => {}; + const expected = 'EventTarget does not have `error` event semantics'; + let actual; + try { + await once(emitter, 'error'); + } catch (error) { + actual = error; + } + strictEqual(emitter.listenerCount('error'), 0); + strictEqual(expected, actual); } Promise.all([ @@ -110,5 +142,7 @@ Promise.all([ catchesErrors(), stopListeningAfterCatchingError(), onceError(), - onceWithEventTarget() + onceWithEventTarget(), + onceWithEventTargetTwoArgs(), + onceWithEventTargetError(), ]).then(common.mustCall()); From 950459386c227cad9beda03a469e1ee456f69dc3 Mon Sep 17 00:00:00 2001 From: Jenia Date: Sun, 8 Sep 2019 21:54:32 +0300 Subject: [PATCH 05/12] events: update docs --- doc/api/events.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/doc/api/events.md b/doc/api/events.md index 3b92a04ca582a2..e748fdb314196a 100644 --- a/doc/api/events.md +++ b/doc/api/events.md @@ -703,11 +703,15 @@ added: v11.13.0 * `name` {string} * Returns: {Promise} -Creates a `Promise` that is resolved when the `EventEmitter` emits the given +Creates a `Promise` that is fulfill when the `EventEmitter` emits the given event or that is rejected when the `EventEmitter` emits `'error'`. The `Promise` will resolve with an array of all the arguments emitted to the given event. +Note: This method is intentionally generic and works with the web platform +[EventTarget](WHATWG-EventTarget) interface, which have no special +`'error'` event semantics and do not listen to the `'error'` event. + ```js const { once, EventEmitter } = require('events'); @@ -745,4 +749,5 @@ run(); [`fs.ReadStream`]: fs.html#fs_class_fs_readstream [`net.Server`]: net.html#net_class_net_server [`process.on('warning')`]: process.html#process_event_warning +[WHATWG-EventTarget](https://dom.spec.whatwg.org/#interface-eventtarget) [stream]: stream.html From 3526b463606229b4f899e46418768777cce0bc1e Mon Sep 17 00:00:00 2001 From: Jenia Date: Mon, 9 Sep 2019 19:48:53 +0300 Subject: [PATCH 06/12] events: docs update --- doc/api/events.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/doc/api/events.md b/doc/api/events.md index e748fdb314196a..2f449981585924 100644 --- a/doc/api/events.md +++ b/doc/api/events.md @@ -703,7 +703,7 @@ added: v11.13.0 * `name` {string} * Returns: {Promise} -Creates a `Promise` that is fulfill when the `EventEmitter` emits the given +Creates a `Promise` that is fulfilled when the `EventEmitter` emits the given event or that is rejected when the `EventEmitter` emits `'error'`. The `Promise` will resolve with an array of all the arguments emitted to the given event. @@ -739,7 +739,7 @@ async function run() { run(); ``` - +[WHATWG-EventTarget](https://dom.spec.whatwg.org/#interface-eventtarget) [`--trace-warnings`]: cli.html#cli_trace_warnings [`EventEmitter.defaultMaxListeners`]: #events_eventemitter_defaultmaxlisteners [`domain`]: domain.html @@ -749,5 +749,4 @@ run(); [`fs.ReadStream`]: fs.html#fs_class_fs_readstream [`net.Server`]: net.html#net_class_net_server [`process.on('warning')`]: process.html#process_event_warning -[WHATWG-EventTarget](https://dom.spec.whatwg.org/#interface-eventtarget) [stream]: stream.html From 6f13b524fed54b63b75063568ae895d03c176c59 Mon Sep 17 00:00:00 2001 From: Jenia Date: Tue, 10 Sep 2019 22:21:47 +0300 Subject: [PATCH 07/12] events: code review updates --- lib/events.js | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/lib/events.js b/lib/events.js index 678db2bfa98d75..8ffcfd48ca5efb 100644 --- a/lib/events.js +++ b/lib/events.js @@ -497,7 +497,7 @@ function unwrapListeners(arr) { function once(emitter, name) { return new Promise((resolve, reject) => { - if (typeof emitter.addEventListener === 'undefined') { + if (typeof emitter.addEventListener !== 'function') { const eventListener = (...args) => { if (errorListener !== undefined) { emitter.removeListener('error', errorListener); @@ -525,21 +525,13 @@ function once(emitter, name) { return; } - if (typeof emitter.addEventListener === 'function') { - // EventTarget does not have `error` event semantics like Node - // EventEmitters, we do not listen to `error` events here. - if (name === 'error') { - reject('EventTarget does not have `error` event semantics'); - } - - emitter.addEventListener( - name, - (...args) => { resolve(args); }, - { once: true } - ); - return; - } - - throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter); + // 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; }); } From 646ae696bdebda41fe8084913c70ea034f2660b4 Mon Sep 17 00:00:00 2001 From: Jenia Date: Tue, 10 Sep 2019 22:23:13 +0300 Subject: [PATCH 08/12] events: update test --- test/parallel/test-events-once.js | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/test/parallel/test-events-once.js b/test/parallel/test-events-once.js index abfcfab8518945..d999890bc74374 100644 --- a/test/parallel/test-events-once.js +++ b/test/parallel/test-events-once.js @@ -123,17 +123,24 @@ async function onceWithEventTargetTwoArgs() { } async function onceWithEventTargetError() { - const emitter = new EventEmitter(); - emitter.addEventListener = () => {}; - const expected = 'EventTarget does not have `error` event semantics'; - let actual; - try { - await once(emitter, 'error'); - } catch (error) { - actual = error; - } + 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); + } + }); + }(); + + const expected = new Error('kaboom'); + process.nextTick(() => { + emitter.emit('error', expected); + }); + + const [err] = await once(emitter, 'error'); + strictEqual(err, expected); strictEqual(emitter.listenerCount('error'), 0); - strictEqual(expected, actual); } Promise.all([ From b8d9389ae18a6466f8f92316c8c8de67b877e6aa Mon Sep 17 00:00:00 2001 From: Jenia Brook Date: Tue, 10 Sep 2019 22:44:08 +0300 Subject: [PATCH 09/12] events: remove redundant return Co-Authored-By: mscdex --- lib/events.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/events.js b/lib/events.js index 8ffcfd48ca5efb..9073591cb0974d 100644 --- a/lib/events.js +++ b/lib/events.js @@ -532,6 +532,5 @@ function once(emitter, name) { (...args) => { resolve(args); }, { once: true } ); - return; }); } From 510270ae38cbed6a01ae60e4f102d87ed165c85d Mon Sep 17 00:00:00 2001 From: Jenia Date: Wed, 11 Sep 2019 20:03:17 +0300 Subject: [PATCH 10/12] events: check first for EventTarget --- lib/events.js | 62 +++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/lib/events.js b/lib/events.js index 9073591cb0974d..f6e4996d20b40f 100644 --- a/lib/events.js +++ b/lib/events.js @@ -497,40 +497,40 @@ function unwrapListeners(arr) { function once(emitter, name) { return new Promise((resolve, reject) => { - if (typeof emitter.addEventListener !== '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); + 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) { + 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(name, eventListener); - return; + emitter.once('error', errorListener); } - // 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 } - ); + emitter.once(name, eventListener); }); } From 126494c96c3f251928f9bb97cfeb44e9f878d320 Mon Sep 17 00:00:00 2001 From: Jenia Brook Date: Thu, 12 Sep 2019 10:39:02 +0300 Subject: [PATCH 11/12] Update doc/api/events.md Co-Authored-By: James M Snell --- doc/api/events.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/events.md b/doc/api/events.md index 2f449981585924..27997cb987fbce 100644 --- a/doc/api/events.md +++ b/doc/api/events.md @@ -708,7 +708,7 @@ event or that is rejected when the `EventEmitter` emits `'error'`. The `Promise` will resolve with an array of all the arguments emitted to the given event. -Note: This method is intentionally generic and works with the web platform +This method is intentionally generic and works with the web platform [EventTarget](WHATWG-EventTarget) interface, which have no special `'error'` event semantics and do not listen to the `'error'` event. From 0cf3e5dd07f7555474ecf72c5ef65f3ad385159e Mon Sep 17 00:00:00 2001 From: Jenia Date: Sat, 14 Sep 2019 18:00:56 +0300 Subject: [PATCH 12/12] events: update tests Implemented EventTargetMock instead extends EventEmitter. --- test/parallel/test-events-once.js | 93 +++++++++++++++++++------------ 1 file changed, 58 insertions(+), 35 deletions(-) diff --git a/test/parallel/test-events-once.js b/test/parallel/test-events-once.js index d999890bc74374..fea143f5877cc7 100644 --- a/test/parallel/test-events-once.js +++ b/test/parallel/test-events-once.js @@ -4,6 +4,53 @@ 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; + }; +} + async function onceAnEvent() { const ee = new EventEmitter(); @@ -85,62 +132,38 @@ async function onceError() { } 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); - } - }); - }(); + const et = new EventTargetMock(); process.nextTick(() => { - emitter.emit('myevent', 42); + et.dispatchEvent('myevent', 42); }); - const [ value ] = await once(emitter, 'myevent'); + const [ value ] = await once(et, 'myevent'); strictEqual(value, 42); - strictEqual(emitter.listenerCount('myevent'), 0); + strictEqual(Reflect.has(et.events, 'myevent'), false); } async function onceWithEventTargetTwoArgs() { - 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); - } - }); - }(); + const et = new EventTargetMock(); process.nextTick(() => { - emitter.emit('myevent', 42, 24); + et.dispatchEvent('myevent', 42, 24); }); - const value = await once(emitter, 'myevent'); + const value = await once(et, 'myevent'); deepStrictEqual(value, [42, 24]); } async function onceWithEventTargetError() { - 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); - } - }); - }(); + const et = new EventTargetMock(); const expected = new Error('kaboom'); process.nextTick(() => { - emitter.emit('error', expected); + et.dispatchEvent('error', expected); }); - const [err] = await once(emitter, 'error'); + const [err] = await once(et, 'error'); strictEqual(err, expected); - strictEqual(emitter.listenerCount('error'), 0); + strictEqual(Reflect.has(et.events, 'error'), false); } Promise.all([