laravel / prompts

Beautiful and user-friendly forms for your command-line PHP applications.
https://laravel.com/docs/prompts
MIT License
533 stars 94 forks source link

Stream output while displaying spinner #85

Closed joetannenbaum closed 7 months ago

joetannenbaum commented 1 year ago

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:

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...',
);

https://github.com/laravel/prompts/assets/2702148/e4b21ac2-a1eb-4d67-92cc-215f34f66fd2

Issues I'm aware of/concerns I have:

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

joetannenbaum commented 1 year ago

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 commented 1 year ago

@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 commented 8 months ago

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

joetannenbaum commented 8 months ago

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 commented 7 months ago

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.