Support NodeJS Event Emitters in fromEvent extra #73
Conversation
Add support for stream generation from EventEmitters. fromEvent will accept as a first argument either a DOMElement or an EventEmitter. When providing fromEvent with an EventEmitter, the third argument is always ignored. Internally, include an additional Producer type within fromEvent to support type checking. Related to staltz#65.
Revert indentation change to remain consistent with project style.
|
Really good PR. I'd fix just some minor issues, usually I do this myself after merging, but you seem willing to do that. I'll comment inline. 👍 |
| import {Stream, InternalProducer, InternalListener} from '../core'; | ||
| /// <reference path="../../typings/globals/node/index.d.ts" /> | ||
| import { EventEmitter } from 'events'; | ||
| import { Stream, InternalProducer, InternalListener } from '../core'; |
There was a problem hiding this comment.
Code style: we use no spaces surrounding curly braces. So, {EventEmitter}.
|
PS: I'm fine with merging and doing the adjustments myself, so let me know what you prefer. |
|
Thank you, @staltz. I'll fix these up. |
Introduce style changes to enforce project source consistency. Adjust type checking within the fromEvent function. Specifically, duck type the first argument rather than rely on `instanceof` or `typeof` guards when deciding which producer type to instantiate. * Remove whitespace to align with project style requirements. * Remove destructuring assignment in producer class methods. * Update documentation to include both uses of the extra. * Remove whitespaces and semicolons from example markdown. * Refactor producer instantiation guards to implement duck-typing rather than `instanceof`/`typeof` checking. * Remove unused import from tests * Reorder assertion equality arguments. * Rename parameter in fromEvent documentation (`eventType`>`eventName`)
|
Apologies for the whitespace issues: I had my linter disabled. I've done my best to address each of your comments. Regarding the type guards inside the extra, I used Rx as the model. The guards are slightly different between 4.x and 5.x-beta, so I implemented the one I considered simpler. I understand that the name, internal logic, and implementation of that guard might merit a revisit in the future. For now, though, it should be just as robust as |
| } | ||
|
|
||
| function isEmitter(element: any): boolean { | ||
| return element.addListener; |
There was a problem hiding this comment.
addListener and also emit, just because xstream Streams also have addListener. :)
Check for both `addListener` and `emit` methods on an element supplied to fromEvent. This ensures that the function will not attempt to invoke the `addListener` method on an xstream stream instance with incorrect arguments. Instead, this scenario will throw a TypeError: 'this.node.addEventListener is not a function'
|
Thanks a lot! Merged. Will release a new version soon. |
Add support for stream generation from EventEmitters.
Description
fromEventwill accept as a first argument either a DOMElement or an EventEmitter. When providingfromEventwith an EventEmitter, the third argument (useCapture) is always ignored.There are two potentially outstanding concerns.
First: This is my first contribution to xstream and I'm not familiar with how the documentation is built. I've updated JSDOC comments with new examples, but it's possible that my comments don't work with the documentation generator.
Second: Parity with RxJS and Most.js should probably involve a change to the third parameter here. I believe they support an object with configuration options, rather than a boolean. I'm not certain if that work would be a requirement to merge, but a brief discussion probably couldn't hurt. I'm happy to update the implementation to accommodate any updated signatures.
Motivation and Context
This should resolve #65.
How Has This Been Tested?
Tests have been split between the two possible uses of the extra and are passing.
Checklist: