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

[5.x] Run commands inside the spin function. #340

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

xiCO2k
Copy link
Contributor

@xiCO2k xiCO2k commented May 27, 2024

This PR adds the capability to have the spin function from Laravel Prompts.

New vs Old

installer-spin.mov

And of course you can still pass the -v and have the exact same output as before.

Cheers.

@mpociot
Copy link
Contributor

mpociot commented May 28, 2024

The spinner can also be used without the pcntl extension installed, as it will render a static spinner in this case: https://github.com/laravel/prompts/blob/main/src/Spinner.php#L50-L52

I would personally prefer it if the message would give a hint at what's currently being installed. Right now it just always says "Installing…" and the user doesn't know what's happening.

Also when some of the commands fail, they fail silently. For example when trying to install NPM dependencies for Breeze with an unsupported Node/NPM version.

@xiCO2k
Copy link
Contributor Author

xiCO2k commented May 28, 2024

I did notice that for the Spinner but I think on the case of the installer it makes more sense to only use it when we have the non-static version.

For the showing what is currently being installed, I did though about that, but I think we can work on that on a future PR after this one.

For the next one, I would divide each command into its own process, and for that case we can have better error messages and we can say "npm install", "composer install", etc.

Thanks for the review @mpociot.

@taylorotwell
Copy link
Member

@xiCO2k awesome - glad you sent this over.

For the next one, I would divide each command into its own process, and for that case we can have better error messages and we can say "npm install", "composer install", etc.

Can we go ahead and do this before merging?

@xiCO2k
Copy link
Contributor Author

xiCO2k commented May 28, 2024

On it!!!

@xiCO2k
Copy link
Contributor Author

xiCO2k commented May 28, 2024

@mpociot can you give it a try and let me know what you think?

Thanks.

@xiCO2k xiCO2k changed the title [1.x] Run commands inside the spin function. [5.x] Run commands inside the spin function. May 31, 2024
@taylorotwell
Copy link
Member

@xiCO2k some conflicts here.

@xiCO2k
Copy link
Contributor Author

xiCO2k commented Jun 3, 2024

Yep will handle it this afternoon

@xiCO2k
Copy link
Contributor Author

xiCO2k commented Jun 3, 2024

ready @taylorotwell

@taylorotwell
Copy link
Member

Drafting this for a bit while I think about it.

@taylorotwell taylorotwell marked this pull request as draft June 17, 2024 14:58
@osbre
Copy link

osbre commented Aug 25, 2024

Unpopular opinion: seeing the "behind the scenes" of installing dependencies is helpful to get familiar with the ecosystem, and allows them to see the progress or when it gets stuck due to the network.

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.

4 participants