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

Spinner runs $callback() twice if statically rendered #65

Closed
brianclogan opened this issue Sep 11, 2023 · 4 comments · Fixed by #66
Closed

Spinner runs $callback() twice if statically rendered #65

brianclogan opened this issue Sep 11, 2023 · 4 comments · Fixed by #66
Assignees
Labels

Comments

@brianclogan
Copy link

Laravel Prompts Version

0.1.6

Laravel Version

10.1.2

PHP Version

8.1.4

Operating System & Version

Ubuntu 20.04 via WSL

Terminal Application

Windows Terminal

Description

First time bug submitter for OSS, if I need to add any more detail let me know!

When using Spinner (not documented yet, found when viewing source) if rendered statically, it will run $callback() twice.

It appears in the Spinner.php class, in the spin() method it calls it here:

if (! function_exists('pcntl_fork')) {
    $this->renderStatically($callback);
}
protected function renderStatically(Closure $callback): mixed
    {
        $this->static = true;
        $this->render();
        return $callback();
    }

And then regardless if it ran via the renderStatically() method, it will run it again in the try block.

Steps To Reproduce

Create a spinner, and have it call statically. Add a echo to the function called, and it will write it to terminal twice if the spinner is rendered statically, only once if rendered normally.

@brianclogan
Copy link
Author

My temp fix locally was just to set $result = null at the start of the spin() method, and set it if rendered statically, and do a check. Don't know if that is the best fix, but it's a working one for me locally.

@jessarcher jessarcher self-assigned this Sep 12, 2023
@jessarcher jessarcher added the bug label Sep 12, 2023
@jessarcher
Copy link
Member

It looks like a return got dropped somewhere along the way.

I couldn't replicate your exact scenario, though. For me, it has a fatal error when it erroneously tries to call pcntl_fork. I tested by replacing all occurrences of pcntl_fork with Xpcntl_fork to simulate the function not existing.

@brianclogan
Copy link
Author

brianclogan commented Sep 12, 2023

I guess the reason it would do that though is due to the return not happening sooner, which would force it to call twice if pcntl is installed. I am forcing the static renderer to run if it is on CI, to prevent the logs from filling with a ton of updates on the spinner using a custom Spinner class. Realistically, the only difference is adding another check to the renderStatically() if statement, but if it's not returning then, that would cause the issue I was experiencing.

Now time to modify my class on my local version. Thanks so much for being so fast to respond! Love the work you have done here, and already tinkering with the themes and everything. :)

@jessarcher
Copy link
Member

Oh, understood! If you're manually triggering the static mode, then I guess #66 won't help - depending on how you're doing it.

I'm currently working on a "non-interactive" mode for Prompts - mostly to prevent prompting for user input when it's not an interactive terminal session, without needing to fall back to Symfony. We could look at changing the spinner behaviour when non-interactive as well.

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

Successfully merging a pull request may close this issue.

2 participants