lorisleiva / laravel-actions

⚡️ Laravel components that take care of one specific task
https://laravelactions.com
MIT License
2.52k stars 124 forks source link

Feat/add service contract #191

Closed llewellyn-kevin closed 2 years ago

llewellyn-kevin commented 2 years ago

This PR adds a contract that can be extended for any actions in a user's application. This allows for easy registration of any action using the full service pattern. For example:

interface DoesSomething extends BaseActionContract
{
    public function handle(): string;
}

class DoSomething implements DoesSomething
{
    use AsAction;

    // ...
}

class LivewireComponent extends Component
{
    public function render()
    {
        return app(DoesSomething::class)->run();
    }
}

The base action contract will automatically add every single method from the AsAction concern to the interface. This is not strictly necessary for the service container to work, but it does ensure that when the interface is consumed static analysis will be able to recognize what methods are available to the service. In this example, calling the run method would work without the BaseActionContract; but phpstorm, intellephense, and phpstan would all be pretty upset about it.

I have already used this pattern in existing applications, but that means any updates to any of the function headers will break my contracts so it would be useful to have something in the repo itself. I added a test to help keep up the contract with any changes that need to be made to the concern, but I understand if this is too much of a pain to keep up with. I thought it might be helpful to other people, but am open to feedback for how to streamline.

(In that vain, I could not for the life of me find a simple method to check if one class is capable of implementing an interface. Seems like there should be a way to do that. Just implementing the interface caused an exception to be thrown before a test could be run. 🤔 I ended up writing a massive to string interpolator to reflect the classes and manually compare the method declarations.)

Incidentally, aside from the minor maintenance cost of keeping the contract in sync, this should be an easy minor version update. It is completely tested and completely non-breaking. It changes nothing about usage for anyone who does not want to use it but would be a life-saver for those who do 😍

Wulfheart commented 2 years ago

Personally I am not a fan of it but it might be that I don’t understand it completely. Why not just inject the action itself?

lorisleiva commented 2 years ago

Hi there 👋

Thank you for taking the time to write this PR.

I have to say though, I agree with Alex. I'm not entirely convinced about the value this bring to project.

If the goal is to make IDEs happy, there are already a bunch of tools out there that I feel provide a better developer experience then the proposed solution.

I'm curious to understand why you'd want to use a base interface instead of them.

lorisleiva commented 2 years ago

Closing this for the following reasons:

Feel free to open a new PR with a clear description of why this would bring value to the project.

Thank you for your understanding.

llewellyn-kevin commented 2 years ago

Hey, thanks for following up @lorisleiva. I meant to close this myself after the initial feedback--sorry for leaving it unattended for so long. Work got crazy busy right after I sent this and I lost track of it.

My main bias is towards solving things within the structures given by the language itself, rather than additional tooling. So an interface feels more natural to me than installing a plugin because it works at the language level, so any environment or static analyzer with a PHP LSP will be happy with my code without any additional help.

But that's primarily a preference thing, so I thought I would share as an option; and if it's not shared I understand not wanting to introduce the cost of maintenance. Thanks for y'alls feedback and providing a great package, it's been a tremendous help!