Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Add "array" and "error" options #1

Merged
merged 1 commit into from
Aug 24, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,26 @@ The name of the event you want to watch.

#### options

##### array

Type: `boolean`
Default: `undefined`

This option controls how a listener's parameters are resolved by
its promise.

If true, the parameters are resolved as an array. If false,
the first parameter is resolved. If undefined (the default),
an array is resolved if there's more than one parameter;
otherwise, the first parameter is resolved.

##### error

Type: `string`
Default: `"error"`

The name of the event which rejects the promise.

##### ignoreErrors

Type: `boolean`
Expand Down
30 changes: 20 additions & 10 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ var Bluebird = require('bluebird')

// Faster `Function.bind()`.
function bind (fn, ctx) {
return function bindedFunction () {
return function boundFunction () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks :)

return fn.apply(ctx, arguments)
}
}
Expand All @@ -21,8 +21,10 @@ var toArray = Array.from || (function (slice) {

// ===================================================================

function eventToPromise (emitter, event, opts) {
var ignoreErrors = opts && opts.ignoreErrors
function eventToPromise (emitter, event, _opts) {
var opts = _opts || {}
var ignoreErrors = opts.ignoreErrors
var errorEvent = opts.error || 'error'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think opts.error could be null to ignore error handling, what's your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that could work (assuming it doesn't replace ignoreErrors).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should remove ignoreErrors in favor of null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do, if you don't mind a breaking change :-) I personally prefer an explicit option, though, even if it is slightly "longer", since it means I don't have to add a comment explaining that error: null means "ignore errors" :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough, lets keep it like that.


return new Bluebird(function (resolve, reject) {
var addListener =
Expand All @@ -44,7 +46,7 @@ function eventToPromise (emitter, event, opts) {

cleanUp = function cleanUp () {
removeListener(event, eventListener)
errorListener && removeListener('error', errorListener)
errorListener && removeListener(errorEvent, errorListener)
}
} else {
cleanUp = noop
Expand All @@ -53,20 +55,28 @@ function eventToPromise (emitter, event, opts) {
function eventListener () {
cleanUp()

if (arguments.length < 2) {
resolve(arguments[0])
} else {
resolve(toArray(arguments))
if ('array' in opts) {
if (opts.array) {
resolve(toArray(arguments))
} else {
resolve(arguments[0])
}
} else { // legacy behaviour for backwards compatibility
if (arguments.length < 2) {
resolve(arguments[0])
} else {
resolve(toArray(arguments))
}
}
}
emitter.on(event, eventListener)
addListener(event, eventListener)

if (!ignoreErrors) {
errorListener = function errorListener (error) {
cleanUp()
reject(error)
}
emitter.on('error', errorListener)
addListener(errorEvent, errorListener)
}
}).bind(emitter)
}
Expand Down
55 changes: 52 additions & 3 deletions index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ var EventEmitter = require('events').EventEmitter
// ===================================================================

// TODO:
// - Forcing the parameters to be forwarded in an array for
// consistency and predictability.
// - Maybe handling multiple events.
// - Tests the different way to add/remove an event listener.

Expand Down Expand Up @@ -81,6 +79,57 @@ describe('event-to-promise', function () {

// -----------------------------------------------------------------

describe('array option', function () {
it('array: false: fowards the first parameter', function () {
var param0 = {}
var param1 = {}

var promise = eventToPromise(emitter, 'foo', { array: false })
emitter.emit('foo', param0, param1)

return promise.then(function () {
expect(arguments).to.have.length(1)
expect(arguments[0]).to.equal(param0)
})
})

it('array: true: fowards all parameters as an array', function () {
var param0 = {}
var param1 = {}

var promise = eventToPromise(emitter, 'foo', { array: true })
emitter.emit('foo', param0, param1)

return promise.then(function (values) {
expect(values).to.have.length(2)
expect(values[0]).to.equal(param0)
expect(values[1]).to.equal(param1)
})
})
})

// -----------------------------------------------------------------

describe('error option', function () {
it('handles a custom error event', function () {
var error = new Error()

var promise = eventToPromise(emitter, 'foo', { error: 'test-error' })
emitter.emit('test-error', error)

return promise.then(
function () {
expect('should not be executed').to.be.true
},
function (err) {
expect(err).to.equal(error)
}
)
})
})

// -----------------------------------------------------------------

it('handles error event', function () {
var error = new Error()

Expand All @@ -101,7 +150,7 @@ describe('event-to-promise', function () {
var error = new Error()

// Node requires at least one error listener.
emitter.on('error', function noop () {})
emitter.once('error', function noop () {})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's called once, after which we have no further use for it. Why would you not use once here?

If there was a bug in this library that caused the error event to be fired twice, once would expose it, while on would silently swallow it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean a bug in the test, because the library does not emit any events.

But fair point.


var promise = eventToPromise(emitter, 'foo', {
ignoreErrors: true
Expand Down