livewire icon indicating copy to clipboard operation
livewire copied to clipboard

Features/Computed: Add support for DI on computed properties

Open ryangjchandler opened this issue 1 year ago • 7 comments

Review the contribution guide first at: https://livewire.laravel.com/docs/contribution-guide

1️⃣ Is this something that is wanted/needed? Did you create a discussion about it first?

Yes – discussion here: https://github.com/livewire/livewire/discussions/6967

2️⃣ Did you create a branch for your fix/feature? (Main branch PR's will be closed)

Yes.

3️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.

No,

4️⃣ Does it include tests? (Required)

Yes.

5️⃣ Please include a thorough description (including small code snippets if possible) of the improvement and reasons why it's useful.

Currently it's not possible to resolve parameters from the container on computed property methods. This is inconsistent with actions which do support DI parameters.

This pull request adds backwards compatible support for this behaviour by-way of a new inject argument on the Computed attribute.

This will change how computed properties are invoked and call the method through Laravel's container instead.

Here's what you need to do at the minute:

#[Computed]
function foo() {
	$dep = app(MyDep::class);

	return $dep->something();
}

And with this change:

#[Computed(inject: true)]
function foo(MyDep $dep) {
	return $dep->something();
}

ryangjchandler avatar Oct 21 '24 13:10 ryangjchandler

Thanks @ryangjchandler - wondering if we can add this functionality without it being a breaking change.

If we have to have an attribute called "inject", at that point it's already verbose enough to just use something like app() right?

Anyhow, because nothing can currently be passed into a #[Computed] method, I'm thinking we're safe. Let me know what you think.

calebporzio avatar Oct 21 '24 15:10 calebporzio

Yeah, the inject parameter on the attribute was more of a "let's be super safe and certain that there won't be any breaking changes" rather than a necessity to avoid breaking changes.

I think we can remove it and everything will still work as expected – like you say, nothing can be passed through to a computed property anyway.

Let me know and I'll remove the attribute parameter etc @calebporzio.

ryangjchandler avatar Oct 21 '24 15:10 ryangjchandler

why not somethink like this

#[Computed]
function foo(#[Resolve] MyDep $dep) {
	return $dep->something();
}

similar to https://laravel.com/docs/11.x/container#contextual-attributes

but overall im more team resolve() / app()

nuernbergerA avatar Oct 21 '24 18:10 nuernbergerA

@nuernbergerA Doing this would required manually handling the reflection of parameters, etc.

ryangjchandler avatar Oct 22 '24 12:10 ryangjchandler

@calebporzio I've updated code to always run computed property methods through the container using Livewire's own ImplicitlyBoundMethod class.

I'm not super clued up on the differences with Laravel's BoundMethod class, so can change if need be.

ryangjchandler avatar Oct 22 '24 12:10 ryangjchandler

Thanks @ryangjchandler - I like this. Quick thought: I think your implementation won't work when computed properties are protected methods because the removal of invade().

calebporzio avatar Oct 22 '24 18:10 calebporzio

@calebporzio Good spot, I'll go back and add a test for protected methods too 👍

ryangjchandler avatar Oct 22 '24 21:10 ryangjchandler