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

Pre & Post Build Command Support #167

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

PeteBishwhip
Copy link
Member

@PeteBishwhip PeteBishwhip commented Feb 18, 2025

Config key defaults provided in NativePHP/laravel#496

This pull request introduces a new trait for handling pre- and post-processing commands in the build process, along with updates to the BuildCommand class and associated tests. The most important changes include adding the HasPreAndPostProcessing trait, updating the BuildCommand to use this trait, and adding tests to ensure the trait works correctly.

New Trait for Pre- and Post-Processing:

  • src/Traits/HasPreAndPostProcessing.php: Introduced the HasPreAndPostProcessing trait to handle pre- and post-processing commands during the build process. This includes methods for executing commands and retrieving configuration.

Updates to BuildCommand:

  • src/Commands/BuildCommand.php: Updated the BuildCommand class to use the new HasPreAndPostProcessing trait and added calls to preProcess and postProcess methods in the handle method. [1] [2] [3]

Tests for the New Trait:

Development Dependency:

  • composer.json: Added mockery/mockery as a development dependency for testing purposes.

Closes #493

@PeteBishwhip PeteBishwhip added the enhancement New feature or request label Feb 18, 2025
@PeteBishwhip PeteBishwhip self-assigned this Feb 18, 2025
@PeteBishwhip PeteBishwhip marked this pull request as ready for review February 18, 2025 14:04
@PeteBishwhip PeteBishwhip requested a review from a team February 18, 2025 14:06
@gwleuverink
Copy link
Contributor

Great work on this as always Pete!

I've added two comments, but honestly this comes down to preference I think. If @SRWieZ is okay with it I am too.

I have a question though. I might not be understanding fully how this works so please bear with me.

Isn't this a question of looping through all the prebuild & postbuild commands & running them? The clever function that returns a function I saw does seem to overcomplicate it a bit in my eyes.

Can you elaborate on why it works like this?

@PeteBishwhip
Copy link
Member Author

Updated, refactored and simplified. The scope creep was real.

Copy link
Contributor

@gwleuverink gwleuverink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

One thing to consider. Should we cancel the build when a process fails?

Comment on lines +38 to +44
->with([
// Empty class with the HasPreAndPostProcessing trait
new class
{
use HasPreAndPostProcessing;
},
]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful!

@PeteBishwhip
Copy link
Member Author

Perfect!

One thing to consider. Should we cancel the build when a process fails?

I did consider this and thought it could be creeping. It may also be that you're fine with things failing in the pre or post steps.

Definitely discussion worthy and can be revisited if decided we should.

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

Successfully merging this pull request may close these issues.

[Feature] Provide helper option to BuildCommand to run asset build prior to native build
2 participants