open-cli-tools / concurrently

Run commands concurrently. Like `npm run watch-js & npm run watch-less` but better.
https://www.npmjs.com/package/concurrently
MIT License
6.98k stars 227 forks source link

Ensuring completion-listener.ts listen to a single close event emitte… #464

Closed odeadglaz closed 5 months ago

odeadglaz commented 7 months ago

Currently, the CompletionListener sets a bufferCount upon commands close streams to conclude once commands are finished and then evaluates the exit code and determines the success/fail conditions.

Continued to the issue brought here, calling manually Command - kill/start API's manually, causes a bug where the CompletionListener resolves before all commands had exited ( as a single command can emit multiple close events ).

This PR changes the behaviour to take a single emit event from each command in order to conclude termination.

Disclaimer: Potentially I would love to find a way to take the last exit command emitted from each Command tho I couldn't find a way would appreciate any suggestion 👍 mine while I kept things the same.

Side effects

Resolves: https://github.com/open-cli-tools/concurrently/issues/463

coveralls commented 7 months ago

Coverage Status

coverage: 99.324% (+0.002%) from 99.322% when pulling 54818fb809766c5cf63f81c7e1e99c59e3bf9bb3 on odeadglaz:main into fd21485203fadaf7a40ef6145cea23fece23d414 on open-cli-tools:main.

odeadglaz commented 7 months ago

Rethinking on this, this change includes also an edge case that doesn't work:

---> It will resolve altho the first command is also alive

So I might be going on the wrong direction here would love to here your tought on the initial design of the start/kill API, were they meant to be used this way?

odeadglaz commented 7 months ago

Another option would be a design-level change such as:

Adding a new restart API for command which goes something like:

restart(code? :string) {
    this.state = 'restart'; // New CommandState
    this.kill();
    this.start();
} 

And a modification for Command.start for skipping emitting close event if the command state equals to restart

Would love to hear your mind on it 👍

odeadglaz commented 7 months ago

So how would u recommend proceeding? Do you wish to have this fix for now? (as you said just a slight improvement for the current state)

odeadglaz commented 6 months ago

@gustavohenke Not sure why I'm getting inconsistent behavior in tests, did you encounter this kind of behavior in the past? (locally over my mac I can't seem to reproduce it)