child process - sanity tests and tweaks

[gwleuverink]

Builds on #389

I started adding some tests. Will add some extra things to the ChildProcess api if that's okay with you @simonhamp.

• Stub out tests
• Set a cwd default if none was given
• Add a ChildProcess::artisan() convenience method
• Provide a way to make processes restart when they exit
• Make sure the cmd argument accepts either a string or a array
• Double check argument order to be consistent with other api's

[simonhamp]

I really like where this is going!

[gwleuverink]

I've checked the argument order of existing existing API's for consistency.

The only component I found that refers to a runtime object on the electron process is Window.

Window::resize(400, 300, 'settings');

The order of arguments is mirrored to what we use for the ChildProcess.

ChildProcess::start('alias-for-child', 'sleep 10s');

Flipping those two arguments would be more consistent. And, now I think about it a bit more, the actual command also feels like the more important argument of the two.

Another option to consider is to use a fluent API instead. Most other components do something like the example below:

Dialog::new()
    ->title('Select a file')
    ->open();

Something similar could be achieved with ChildProcess, like we did with the proc_open experiment. But this may be overkill for this scenario. The operations on a process before and after starting it is quite limited.

$loop = ChildProcess::alias('main-loop')
    ->persistent()
    ->artisan('main-loop:start')
    ->message('send to stdin'); // race condition / nonsensical use-case?

My gut tells me to go with flipping the arguments. What do you think @simonhamp?

[gwleuverink]

I can add the option to make a process restart when it exits somewhere here. Do you already have something for this on the electron side @simonhamp?

[simonhamp]

Yep let's flip the args 👍🏼

I can add the option to make a process restart when it exits somewhere here. Do you already have something for this on the electron side @simonhamp?

Yes I do. I think we should let the Electron side handle this for now

[gwleuverink]

I think that's it. Need to do some testing tonight to make sure this integrates with the persistent option on the electron side.

Will mark this as ready for review

[gwleuverink]

If this is okay like this I'll start writing some documentation next

[simonhamp]

Definitely! I've made a start on the docs here: NativePHP/nativephp.com#53

[simonhamp]

these are great improvements. I think it just needs a little more to take it to the next level

[simonhamp]

This is looking excellent