-
Notifications
You must be signed in to change notification settings - Fork 30k
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: add async emitter.when #15204
Changes from 4 commits
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 |
---|---|---|
|
@@ -586,6 +586,54 @@ to indicate an unlimited number of listeners. | |
|
||
Returns a reference to the `EventEmitter`, so that calls can be chained. | ||
|
||
### async emitter.when(eventName[, options]) | ||
<!-- YAML | ||
added: REPLACEME | ||
--> | ||
|
||
* `eventName` {any} The name of the event. | ||
* `options` {Object} | ||
* `prepend` {boolean} True to prepend the handler used to resolve the | ||
`Promise` to the handler queue. | ||
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. Does this even make a difference since any handler is called asynchronously anyway? 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. Likely not. This is also something I've gone back and forth on so I'm happy to pull this back out if it doesn't make sense 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, This would have an effect when registering multiple // `a` will always be printed before `b`
ee.when('foo').then(() => console.log('b'));
ee.when('foo', { prepend: true }).then(() => console.log('a'));
ee.emit('foo'); // `b` will always be printed before `a`
ee.when('foo').then(() => console.log('b'));
ee.when('foo', { prepend: false }).then(() => console.log('a'));
ee.emit('foo'); 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.
Yes, and I would consider relying on promise resolution order a big anti-pattern ;) 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 certainly cannot disagree with that :-) |
||
* Returns: {Promise} | ||
|
||
Creates and returns a `Promise` that is resolved with a one-time event handler. | ||
|
||
```js | ||
const myEmitter = new EventEmitter(); | ||
|
||
const p = myEmitter.when('foo') | ||
.then((context) => { | ||
console.log(context.args); | ||
}); | ||
|
||
myEmitter.emit('foo', 1, 2, 3); | ||
``` | ||
|
||
The `Promise` is resolved with a `context` object that contains two properties: | ||
|
||
* `emitter` {EventEmitter} A reference to the `EventEmitter` instance. | ||
* `args` {Array} An array containing the additional arguments passed to the | ||
`emitter.emit()` function. | ||
|
||
When a `Promise` is created, a corresponding `removeListener` event handler is | ||
registered that will reject the `Promise` if it has not already been resolved. | ||
When rejected in this manner, the rejection `reason` shall be a simple string | ||
equal to `'canceled'`. | ||
|
||
```js | ||
const myEmitter = new EventEmitter(); | ||
|
||
const p = myEmitter.when('foo') | ||
.catch((reason) => { | ||
if (reason === 'canceled') { | ||
// The promise was canceled using removeListener | ||
} | ||
}); | ||
|
||
myEmitter.removeAllListeners(); | ||
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. Missing event name. myEmitter.removeAllListeners('foo'); 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. It's not needed in this case. |
||
``` | ||
|
||
[`--trace-warnings`]: cli.html#cli_trace_warnings | ||
[`EventEmitter.defaultMaxListeners`]: #events_eventemitter_defaultmaxlisteners | ||
[`domain`]: domain.html | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const EventEmitter = require('events'); | ||
|
||
const ee = new EventEmitter(); | ||
|
||
{ | ||
ee.when('foo') | ||
.then(common.mustCall((context) => { | ||
assert.strictEqual(context.emitter, ee); | ||
assert.deepStrictEqual(context.args, [1, 2, 3]); | ||
})) | ||
.catch(common.mustNotCall()); | ||
assert.strictEqual(ee.listenerCount('foo'), 1); | ||
ee.emit('foo', 1, 2, 3); | ||
assert.strictEqual(ee.listenerCount('foo'), 0); | ||
} | ||
|
||
{ | ||
let a = 1; | ||
ee.when('foo') | ||
.then(common.mustCall(() => { | ||
assert.strictEqual(a, 2); | ||
})) | ||
.catch(common.mustNotCall()); | ||
|
||
ee.when('foo', { prepend: true }) | ||
.then(common.mustCall(() => { | ||
assert.strictEqual(a++, 1); | ||
})) | ||
.catch(common.mustNotCall()); | ||
|
||
ee.emit('foo'); | ||
} | ||
|
||
{ | ||
ee.when('foo') | ||
.then(common.mustCall(() => { | ||
throw new Error('foo'); | ||
})) | ||
.catch(common.mustCall((err) => { | ||
assert.strictEqual(err.message, 'foo'); | ||
})); | ||
assert.strictEqual(ee.listenerCount('foo'), 1); | ||
ee.emit('foo'); | ||
assert.strictEqual(ee.listenerCount('foo'), 0); | ||
} | ||
|
||
{ | ||
ee.removeAllListeners(); | ||
ee.when('foo') | ||
.then(common.mustNotCall()) | ||
.catch(common.expectsError({ | ||
code: 'ERR_EVENTS_WHEN_CANCELED', | ||
type: Error, | ||
message: 'The when \'foo\' promise was canceled' | ||
})); | ||
ee.removeAllListeners(); | ||
} | ||
|
||
{ | ||
ee.removeAllListeners(); | ||
assert.strictEqual(ee.listenerCount(0), 0); | ||
const promise = ee.when('foo'); | ||
promise.then(common.mustNotCall()) | ||
.catch(common.expectsError({ | ||
code: 'ERR_EVENTS_WHEN_CANCELED', | ||
type: Error, | ||
message: 'The when \'foo\' promise was canceled' | ||
})); | ||
const fn = ee.listeners('foo')[0]; | ||
assert.strictEqual(fn.name, 'promise for \'foo\''); | ||
assert.strictEqual(fn.promise, promise); | ||
ee.removeListener('foo', fn); | ||
} | ||
|
||
process.on('unhandledRejection', common.mustNotCall()); | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
If it's just returning a Promise, do we really need to add the 'async ' prefix here? None of the examples even utilize async/await.
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 was going back and forth on this. It depends on what (if any) convention we want to set on this. I'm not completely set on it and I expected that if anyone didn't like it then they'd speak up :-)
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.
async/await works for any
Promise
right? If so, I'm not sure we need to explicitly add the prefix in that case.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 do like the
async
prefix.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 like the async prefix too but think that it's important we're consistent with the other method returning promises in the API (util.promisify)