feat(fromEvent): Aggregate multiple arguments #89
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -29,7 +29,9 @@ export class NodeEventProducer implements InternalProducer<any> { | |||
| constructor(private node: EventEmitter, private eventName: string) { } | |||
|
|
|||
| _start(out: InternalListener<any>) { | |||
| this.listener = (e: any) => out._n(e); | |||
| this.listener = (...args: Array<any>) => { | |||
| return (args.length > 1) ? out._n([...args]) : out._n(args[0]); | |||
There was a problem hiding this comment.
I probably don't need to expand [...args] here. @staltz, if this bothers you I'll fix it.
Edit: it bothers me, I'll probably fix this today unless it gets merged.
There was a problem hiding this comment.
Yeah, [...array] is the same as array. It's like x + 1 - 1. :)
There was a problem hiding this comment.
This will also be good information to have in the future, do you prefer amended/rebased stuff for changes like this, or smaller fix commits?
If provided with an EventEmitter, `fromEvent` streams will emit an array
of values when the EventEmitter provides listeners with multiple
arguments.
The previous implementation naively provided only the first argument,
which made `fromEvent` incompatible with event interfaces that rely on
providing multiple arguments to their listeners, such as Node's
`http.Server`.
An alternative solution involved providing selector support. This wasn't
implemented for several reasons.
1. Modifying the function signature for the extra would be a
breaking change.
2. An expanded function signature would become 'crowded', and one of
the architectural goals of xstream is to keep interfaces as small
as they need to be to achieve their goals.
3. Aggregating emitted events into an array is consistent with the
implementation in Most.js, and achieves similar flexibility with Rx
when providing a selector function to `.map()` on a result stream.
`fromEvent` will now emit mixed-types. If consumers are not responsible
for calling `.emit()` on the source emitter, they should implement
appropriate guards to ensure they are dealing with an intended type.
Should resolve staltz#84.
xtianjohns
force-pushed
the
feat-agg-events
branch
from
92c2aad to
e36fd48
Compare
|
Thanks! Merged |
|
Will be released in v5.3.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If provided with an EventEmitter,
fromEventstreams will emit an array of values when the EventEmitter provides listeners with multiple arguments.Description
The previous implementation naively provided only the first argument, which made
fromEventincompatible with event interfaces that rely on providing multiple arguments to their listeners, such as Node'shttp.Server.An alternative solution involved providing selector support. This wasn't implemented for several reasons.
.map()on a result stream.fromEventwill now emit mixed-types. If consumers are not responsible for calling.emit()on the source emitter, they should implement appropriate guards to ensure they are dealing with an intended type.I think that it would be great if
fromEventcould support typed EventEmitters so that downstream consumers could rely on TypeScript to warn them about guards, but EventEmitter isn't a generic type and that would take some rejiggering that I didn't think was warranted to implement this feature.Motivation and Context
This should resolve #84.
Checklist: