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

Stream output while displaying spinner #85

Closed

Conversation

joetannenbaum
Copy link
Contributor

This PR introduces the optional streaming of output while displaying the spinner. Admittedly, it's still a WIP, but I think it's ready enough to get some more eyeballs on it. This is a fresh PR spawned from the conversation in #77.

It includes:

  • The ability to stream output from a process to the terminal while the spinner is showing
  • The ability to change the spinner message as the task is processing

Examples (included a new playground, playground/streaming-spinner.php):

spin(
    function (SpinnerMessenger $messenger) {
        $process = Process::fromShellCommandline('composer update');
        $process->start();

        foreach ($process as $type => $data) {
            if ($process::ERR === $type) {
                $messenger->output($data);
            }
        }

        return 'Callback return';
    },
    'Updating Composer...',
);

spin(
    function (SpinnerMessenger $messenger) {
        foreach (range(1, 50) as $i) {
            $messenger->line("<info>✔︎</info> <comment>Step {$i}</comment>");

            usleep(rand(50_000, 250_000));

            if ($i === 20) {
                $messenger->message('Almost there...');
            }

            if ($i === 35) {
                $messenger->message('Still going...');
            }
        }
    },
    'Taking necessary steps...',
);
CleanShot.2023-09-22.at.22.03.55.mp4

Issues I'm aware of/concerns I have:

  • Spacing issues: Two streaming spinners in a row don't have a space between them, a streaming spinner that occurs right away doesn't have a leading space, a streaming spinner that occurs last doesn't have a trailing space
  • I don't love that we are using the Colors trait and writing the streaming output directly in the Spinner class as opposed to a renderer class
  • SpinnerMessenger isn't the clearest name for the object that is passed as an argument to the task callback, open to suggestions
  • I grabbed the Connection class directly from spatie/fork, not sure what the general ethics/vibes around that are. It's a really simple class, but just wanted to be above board.
  • I haven't included any tests just yet as I'm still finalizing the API

Let me know what you think! Excited about this possibility.

@joetannenbaum joetannenbaum marked this pull request as draft September 23, 2023 02:15
@joetannenbaum
Copy link
Contributor Author

Cleaned this up a bit and got some more consistent output(ish). Right now if you render two streaming spinners back to back, it's consistent.

But if you render a non-spinner before the spinner, the spacing between the output and the line changes, and it also doesn't erase the line at the end.

Brain is a bit mushy at this point, so I'll come back to this later and see if I can clean it up further.

@joetannenbaum
Copy link
Contributor Author

@jessarcher I think this is ready for review.

I know this PR is a bit of a beast, so no rush here. The whole thing is open to conversation and adjustment, I'm not married to the API or implementation. Also, if you decide it just overcomplicates this component, I would also fully understand.

All that being said, in my testing it displays consistently and looks good on my end.

I tried to write tests for it, but realized that all of the streaming output is actually happening in the child process of the fork, so I couldn't figure out how to access that for the test. That's... probably a smell.

If you want to take a look and see if you even like the existing implementation I can work on getting tests that work properly.

@driesvints
Copy link
Member

@joetannenbaum is this still ready for review? Can you mark this as such if so?

@joetannenbaum
Copy link
Contributor Author

Mmmm not sure to be honest. I'll put fresh eyes on this today and see if it's what I want it to be. If not I'll close it out, thanks for the ping.

@joetannenbaum
Copy link
Contributor Author

I still like this idea, but I'm going to close this PR for now. I'd like to take a fresh spin on it in a future PR (pun intended), try to get it a bit cleaner.

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

Successfully merging this pull request may close these issues.

2 participants