Add in the ability for DI onto view models methods

[JamesFreeman]

This PR adds the ability to Dependency Inject against any of the method calls against the ViewModel.

public function getTweets(TwitterService $twitterService)
{
    return $twitterService->get();
}

[JamesFreeman]

Looks like tests are failing here, I'm going to try and look into this ASAP, It would be good to get your feedback on this and how you would potentially implement this?

[brendt]

Thanks for the PR.

I'm not sure whether we want to add this, to be honest. I can see why it's useful, but also fear that it promotes ViewModels with tons of dependencies. On the other hand, it if there's one place where several such dependencies should be allowed, it are view models; since they are the closest to the end user…

So: I'm not sure at the moment.

@sebastiandedeyne we've bee talking about viewmodels lately, what's your opinion?

[sebastiandedeyne]

I don't really have a strong opinion on this.

On the one hand, I like the idea of keeping view models as dumb as possible (move data fetching to the controller). On the other, I like view models that are useable without too much setup outside of them to make them easily composable.

I wouldn't mind merging this in.

[JamesFreeman]

Sounds good, I'll take a look at fixing these tests this evening. How do you guys feel about how I've implemented this so far? Is there a better way of translating a Class Path to container instance?

[brendt]

It looks good, the only thing I'm wondering about is how it works together with view functions ?

[JamesFreeman]

Hey @brendt,

I've pushed up my changes now, It was pretty tricky to get both the callable function calls and the DI method calls working together but I think I've come up with a nice solution. I have created a DI view test to make sure it is working correctly, does this address your concerns on the view method calls?

[brendt]

Can you elaborate how the solution works?

[JamesFreeman]

When the VM is converting the methodToVariable, it will get all of the parameters that are passed in. It will then work out if that variable is typed, if it is it will try to figure out if it's a class and if it is it will resolve that class from the container.

It will then work out if the method is callable by seeing if there is a mismatch in parameter counts.

I wasn't 100% sure on the best way for this to implemented, I'm happy to take some guidance and go back to the drawing board if needed.

[nickfls]

@JamesFreeman will that work with container's $this->app->when(VM::class)->needs(...)>give(..)?

[JamesFreeman]

Laravel should handle that from there side, I'm not really interacting with the container other than passing the class into app();

[brendt]

It will then work out if that variable is typed, if it is it will try to figure out if it's a class and if it is it will resolve that class from the container.

That's not a fool proof, unfortunately. Imagine this method (taken from the readme):

class PostViewModel extends ViewModel
{
    public function formatDate(Carbon $date): string
    {
        return $date->format('Y-m-d');
    }
}

Carbon is a class, but shouldn't be resolved from the container.

The only viable solution I can think of is to introduce attributes:

class PostViewModel extends ViewModel
{
    public function formatDate(
        Carbon $date,
        #[FromContainer] DateFormatter $dateFormatter,
    ): string {
        // …
    }
}

But at this point I'm wondering whether the feature is worth it, especially when the same is already possible with constructor injection.

[JamesFreeman]

I think my code will deal with that situation because it's being passed in via a callable method, I'll spend some time this afternoon adding that Carbon callable instance as a test, at the very least we can get that test merged in.

[JamesFreeman]

I spent some time on this yesterday evening and it looks like it doesn't work correctly. I think the fact that VM has invokable methods makes this sort of implementation near impossible without it being super messy/hacky.

What are your thoughts @brendt?

[brendt]

I really appreciate the effort but think that we've hit a road block here. Maybe we can reconsider it in the next major version or by introducing attributes; but I feel like it's a lot of work for little benefit, especially since DI is possible via the constructor already.

So I'm going to close this one.