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

Clean up API #72

Open
clue opened this issue Jan 15, 2019 · 3 comments
Open

Clean up API #72

clue opened this issue Jan 15, 2019 · 3 comments

Comments

@clue
Copy link
Member

clue commented Jan 15, 2019

The roadmap (#31) currently mentions "clean up API" for the v0.7.0 release, so I'm filing this ticket to discuss possible things that could be cleaned up.

Here's some of the things that I would like to address:

1.) The Process constructor and the start() method form a temporal dependence (you have to call start() exactly once after constructing). Additionally, because creating a process requires creating an instance of Process means that this is really hard to test in higher-level components that dynamically create the command for example (you can't inject a mock for instance, see https://github.com/clue/reactphp-ssh-proxy/blob/7ac2c246a1a5fd86246452d04346092cdd8bb1a7/src/SshProcessConnector.php#L89 for example). As an alternative, I would suggest adding a Launcher (name subject to discussion) which acts as a "factory" to create processes. This solves many of these problems, allows easily creating custom implementations for the points discussed below and also allows simply injecting a mock to higher-level implementations. Example usage:

// concept only
$launcher = new Launcher($loop);
$process = $launcher->launch('ping google.com');

$process->on('exit', 'var_dump');

2.) Inherited file descriptors (#51) are likely not what most use cases want. We should provide a "default launcher" which automatically takes care of closing all file descriptors. This can easily be integrated into the "launcher" concept above and we may or may not want to provide additional options to control this behavior.

3.) Windows support (#67) requires special care even for the easiest use cases. We should provide a "windows launcher" which automatically takes care of setting up I/O redirection for most common use cases. This can easily be integrated into the "launcher" concept above.

4.) The API is quite complex for common use cases. We should provide additional helper methods to cover common use cases. This can easily be integrated into the "launcher" concept above. For example, it's very common you only consume readable data from process output or want to buffer all the data:

// concept only
$launcher = new Launcher($loop);
$stream = $launcher->stream('ping example.com');

$stream->on('data', function ($chunk) {
    echo $chunk;
});


// concept only
$launcher = new Launcher($loop);
$promise = $launcher->buffer('ping example.com');

$promise->then(function ($data) {
    echo $data;
});

Any input is welcome! 👍

@clue clue added this to the v0.7.0 milestone Jan 15, 2019
@loilo
Copy link

loilo commented Jan 16, 2019

Having a Promise-based helper as in (4) would be really useful.

A thing that should be baked in: Such an interface would offer the great opportunity of combining $promise->cancel() with $process->terminate() for really easy premature elimination of long-running processes.

@ghost
Copy link

ghost commented Apr 28, 2019

The only issue with 1) would be that there is no interface to type declare against. With #74 in mind an interface would possibly be required anyway, so the problem of mocking would be solved. To be clear, the "low level" API of directly using a Process should still exist, even when a launcher should be added.

However interfaces should exist so that the implementation can be arbitrarily replaced or customized, without introducing any dependencies or adaptions in code.

@ghost
Copy link

ghost commented Apr 27, 2020

I'd like to pick up 1) + 4) (Launcher concept), are there any specific helper methods that need to exist?

On my list currently:

  • launch ("builds" and starts the process)
  • buffer (launch + buffer stdout stream)
  • stream (should it only capture stdout?)

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

No branches or pull requests

2 participants