Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

JuanCaicedo
Copy link

@JuanCaicedo JuanCaicedo commented Jan 8, 2017

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

  • Adds a -g or --group flag to enable output aggregation
  • No breaking changes

Working scenario

  • If the commands finish in succession (first command finishes first, then second, then third). This works great.

Bugs

  • However, if a previous command runs slower than one of the commands that follow it, the output of the latter command(s) are swallowed up.

Could you use some guidance on

  • How to tackle the bug above
  • How to add tests
  • Code improvements

TODO

  • Update readme to list group option
  • Combine stderr in the same way

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);
Copy link
Author

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

Copy link
Member

Choose a reason for hiding this comment

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

It's okay :)

@JuanCaicedo
Copy link
Author

Tests are failing because of node v0.12, mentioned in https://github.com/kimmobrunfeldt/concurrently/issues/76

@JuanCaicedo
Copy link
Author

Does the bug maybe have to do with the fact that merge is also used on line 280? If I try changing that use concat though, all output of later commands gets swallowed

@JuanCaicedo
Copy link
Author

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

@gustavohenke
Copy link
Member

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?

@JuanCaicedo
Copy link
Author

@gustavohenke thanks for your work, excited to see this project moving forward! Yes, that's what I mean

@gustavohenke
Copy link
Member

gustavohenke commented Jan 27, 2017

Okay, so I might have found a way to fix that problem.

A process with index i needs to buffer until all processes from index 0 to i (inclusive) close.
This means:

  • Process 0 buffers until 0 closes;
  • Process 1 buffers until 0 and 1 close;
  • Process 2 buffers until 0, 1 and 2 close;
  • ...and so on.

Possible with some wise usage of buffer operator together with the close streams.

@gustavohenke
Copy link
Member

Of course, there may be easier/simpler ways. I'm not a heavy user of Rx...

@JuanCaicedo
Copy link
Author

@gustavohenke I have a some more time over these next few weeks to work on this, I'll take another stab at it 😃

@alerkesi
Copy link

Guys, which status for this PR? How can I help something to push this faster?

@JuanCaicedo
Copy link
Author

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants