Support waiting for multiple event emissions

[szmarczak]

Fixes #3

[sindresorhus]

Any reason you went away from using a separate method .multi(), like in your proposal in #3? Is not weird that an option can change the return value of the function? Especially weird that setting resolveImmediately suddenly makes it return an array. For example, let's say someone decides to dynamically generate the value for the count option. It's very easy to forget to account for that if the generated count is suddenly 1, the return type changes.

[szmarczak]

Any reason you went away from using a separate method .multi(), like in your proposal in #3? Is not weird that an option can change the return value of the function?

You use .multi() when you the count is greater than 1. For 1 you don't use multi. But is .multi() needed? It's obvious that if count > 1 it returns an array.

Especially weird that setting resolveImmediately suddenly makes it return an array.

It isn't weird. There's no sense using count: 1, resolveImmediately: true. You use resolveImmediately when you want to access items before you got the count.

const data = await pEvent(emitter, event, {resolveImmediately: true});
// 0s later => []
// 1s later => ['a']
// 2s later => ['a', 'b']
// 3s later => ['a', 'b', 'c']
// 4s later => ['a', 'b', 'c', 'd']

resolveImmediately: true and count: 1:

const data = await pEvent(emitter, event, {count: 1, resolveImmediately: true});
// 0s later => []
// 1s later => ['a']

You can achieve the same by:

const data = await pEvent(emitter, event); // 'a'

For example, let's say someone decides to dynamically generate the value for the count option.

const data = pEvent(emitter, event, {count: Infinity, resolveImmediately: true});
const result = await data;
setTimeout(() => data.cancel(), n);

It's very easy to forget to account for that if the generated count is suddenly 1, the return type changes.

Answer:

It's obvious that if count > 1 it returns an array.

How do you imagine storing a few items in one item? :P You have to use an array.

[szmarczak]

I can do .multi() (then pEvent() would throw if passed count or resolveImmediately option) but there's no need. It requires the count option anyway, right?

[sindresorhus]

I just think the separation would be good. count and resolveImmediately would then be .multi-only options.

[sindresorhus]

It's obvious that if count > 1 it returns an array.

You say that, but is it really obvious when reading the code. If I see one call that has {count: 3} and one that has {count: 1}, I would, from just reading the code, assume both would return an array.

[sindresorhus]

Swift has taught me the value of having a single return value. It's much more predictable and readable.

[sindresorhus]

For example, let's say someone decides to dynamically generate the value for the count option.

const data = pEvent(emitter, event, {count: Infinity, resolveImmediately: true});
const result = await data;
setTimeout(() => data.cancel(), n);

This does not address my concern. You are not generating count dynamically there.

[sindresorhus]

Having had some time to think about this today, I'm even more convinced now that a separate method is the right call. Let's call it .multiple() instead though. No point in shortening it.

[szmarczak]

This does not address my concern. You are not generating count dynamically there.

Do you mean passing a function?

Having had some time to think about this today, I'm even more convinced now that a separate method is the right call. Let's call it .multiple() instead though. No point in shortening it.

Will do :)

[sindresorhus]

Do you mean passing a function?

Yes:

const data = await pEvent(emitter, event, {count: generateRandomCount()});

It's not clear from reading this that data could sometimes be a value and sometimes and array.

[szmarczak]

const data = await pEvent(emitter, event, {count: generateRandomCount()});

do you mean

-const data = await pEvent(emitter, event, {count: generateRandomCount()});
+const data = await pEvent(emitter, event, {count: generateRandomCount});

NEVERMIND, I got it now! :D

(by dynamically first I understood it as passing a function to the count option :P it is just the user won't know if it will be an array or a function, cuz generateRandomCount value may be different, right?)

[sindresorhus]

it is just the user won't know if it will be an array or a function, cuz generateRandomCount value may be different, right?)

Correct

[szmarczak]

Maybe we should introduce it as an option?

pEvent(emitter, 'event', {
	multiple: true,
	count: 2
}); // returns ['a', 'b']

pEvent(emitter, 'event', {
	multiple: false,
	count: 2
}); // waits for two events, but returns only the first one

Also would get rid of these lines of code:

p-event/index.js

Lines 88 to 96 in 23481b5

	options = Object.assign({}, options, {
	count: 1,
	resolveImmediately: false
	});
	const arrayPromise = multiple(emitter, event, options);
	const promise = arrayPromise.then(array => array[0]);
	promise.cancel = arrayPromise.cancel;

[sindresorhus]

I still think it should be a separate method. I considered a multiple option, but then it would conflict with the resolveImmediately option and cause ambiguity with the count option. I just feel it's cleaner to separate them.

waits for two events, but returns only the first one

I don't see the point of this. If you want to wait for two event, but only use the first event, you just do events[0].

Also would get rid of these lines of code:

I'd rather optimize for the public API than internal reducing code.

[szmarczak]

Then the PR's ready to be reviewed :)

[sindresorhus]

You need to update the first paragraphs in the readme, as they assume support for only a single event.

[sindresorhus]

This should provide a use-case for the option, as it's not immediately clear, and also clarify that the returned array will be mutated. when new events are emitted.

[sindresorhus]

This needs to be inside the new Promise or return a Promise.reject(). Methods that return a promise should always do so, even in the failure case.

[szmarczak]

No. You're wrong. The promise executes a task, does not generate it. We need to verify options in the promise generator, we need to verify the options before a task is scheduled.

[szmarczak]

const foo = pThrottle(invaidOptions); // this wouldn't throw
foo (); // this would throw and should NOT

[sindresorhus]

https://twitter.com/jaffathecake/status/638659818996269056

[szmarczak]

Oh, you're right. Sorry, I just got confused (cuz I work on many things at the same time and my mind is mixed up) :p

[sindresorhus]

I don't see the point of not allowing 0 and 1 as count. That just makes it harder to use this API programmatically with unexpected count numbers.

Suggested change

	throw new TypeError('`count` option should be a number greater than 1');
	throw new TypeError('The `count` option should be a positive integer or Infinity');

The count option should also be documented for the above, that it accepts a positive integer or Infinity.

[szmarczak]

Also, why have you removed the article from the test name?

[szmarczak]

Ok, so count >= 0 it is.

[sindresorhus]

Also, why have you removed the article from the test name?

I felt it was moot.

[szmarczak]

Ah, just a shortcut? :P

[sindresorhus]

Perfect 👌

[raymondfeng]

Nitpick: options.batchSize might be a better name for options.count as it hints that we deliver multiple events in batch of the given size. Similarly, pEvent.batch() might be easier to understand.

[mysticatea]

Has this feature released? I have encountered TypeError on pEvent.multiple(), on p-event@2.1.0.

[kevva]

Has this feature released? I have encountered TypeError on pEvent.multiple(), on p-event@2.1.0.

No, not yet. Latest release was in June.

[sindresorhus]

It seems I just forgot to release it. Duuh. Done now.

https://github.com/sindresorhus/p-event/releases/tag/v2.2.0