-
-
Notifications
You must be signed in to change notification settings - Fork 236
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
WIP - Add ability to merge output #79
Conversation
function handleOutput(streams, childrenInfo, source) { | ||
var sourceStreams = _.map(streams, source); | ||
var combinedSourceStream = Rx.Observable.merge.apply(this, sourceStreams); | ||
var combinedSourceStream = combineSourceStreams(this, sourceStreams, config.group); |
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.
Though the config is available as a global, I chose to pass it as a parameter to this new function. I prefer this style, but I'm open to changing it
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.
It's okay :)
Tests are failing because of node v0.12, mentioned in https://github.com/kimmobrunfeldt/concurrently/issues/76 |
Does the bug maybe have to do with the fact that |
My current thought right now is that this has to do with RX not handling back pressure out of the box. Here is a link with more information |
Hi @JuanCaicedo! Thanks for this PR. As you may have noted, I'm getting started here as a maintainer :) The node v0.12 build problem was fixed. If you push something new, the status will be green. Regarding the tests for this feature... if you could emulate the example given at #75, I guess it would be enough. And what do you mean by the output being "swallowed"? Does this means it will never show up? |
@gustavohenke thanks for your work, excited to see this project moving forward! Yes, that's what I mean |
Okay, so I might have found a way to fix that problem. A process with index
Possible with some wise usage of |
Of course, there may be easier/simpler ways. I'm not a heavy user of Rx... |
@gustavohenke I have a some more time over these next few weeks to work on this, I'll take another stab at it 😃 |
Guys, which status for this PR? How can I help something to push this faster? |
@alerkesi Do you want to prototype an approach like @gustavohenke mentioned above? Or if you can think of a different way to do it, that would be good too (I'm also not too experienced with Rx) |
I started work on addressing https://github.com/kimmobrunfeldt/concurrently/issues/75. This change makes this almost possible but it has a bug that I'll detail. instructions. I've made a repo that imports my changes and allows you test them.
Changes
-g
or--group
flag to enable output aggregationWorking scenario
Bugs
Could you use some guidance on
TODO