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

[odeadglaz]

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

• I re-arranged the completion-listener.spec.ts file to all use the same command exit helper
• Allow ipc for spawned commands

Resolves: #463

[coveralls]

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

[odeadglaz]

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

• Starting 2 commands:
• First command manually killed, and then started
• The second command manually killed

---> 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]

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 👍

[gustavohenke]

Restarting is tricky; because each option you set changes the closing conditions of a command, the flow controllers wrap the close observable so that all looks simple on the completion listener side.

Adding a new restart API

I think this could be done for convenience, but not as a way of fixing this issue.

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

This is true, but this PR is at least an improvement over not doing anything.

The optimal solution is probably to retain the last emitted close event of each command until they all exit.

[odeadglaz]

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)

[gustavohenke]

So how would u recommend proceeding?

I think that replacing these lines with Rx.combineLatest() + removing the take(1) above can achieve retaining the last emitted close event.

We can then also fix your edge case from #464 (comment) with a filter() operator that checks if every command is indeed closed.
This means waiting until every command emits another close event.

WDYT?

[odeadglaz]

Yea that sounds legit, at-least the first part, seems like combineLatest can do the job 👍 Ill try to change the implementation accordingly

[odeadglaz]

@gustavohenke I've pushed the changes, the only drawback using the combineLatest is that it actually keep the close events according to the registered stream and not by the actual order they were emitted, So beside accepting your offer I also added a map to sort it via the startingDate of the event, would love to hear if you got any better suggestion here 👍

[gustavohenke]

exitCode is never null here. You need to look at the event's command for the current state

Suggested change

	filter((exitInfos) => exitInfos.every((exitInfo) => exitInfo.exitCode !== null)),
	filter((exitInfos) => exitInfos.every(({ command }) => command.state !== 'started')),

[odeadglaz]

Yea good point 👍 This means to access it we would need to either add it to the CommandInfo which I assume we don't want as it's a public interface or send it explicitly right?

[gustavohenke]

Heya, I've fixed this myself and added an extra test too.

You were right, adding to CommandInfo is not a good idea, but so wasn't right to pass the state in the close event since that's pretty static. Can check commands variable just outside the loop, though.