I like the api.
Personally I would see the hydrate api similar to model attribute modification hence the hydrateAttributeModel method idiom. But external hydrators is a nice separation and probably good for reuse in other components.
I’m curious how your going to implement setting and lazy access if the developer has declared the public variable on the class.
Cool, yeah I’m going to do some hacky stuff, but basically: unset($component->post) will allow a call to $component->post to hit the __get() magic method. Not the most elegant solution, but the API is worth it to me.
I like the computed property (option 2) because that’s cleaner to me. If I want to add eager loading or anything else, this feels very intuitive to me.
Option 1 with Casting, hydrating, and dehydrating is going to confuse a lot of people who don’t understand the full Livewire lifecycle IMO. It’s not immediately clear to me how to use that PostCaster, where the first API is very clear.
Good insight Kevin,
I think I’m coming around to that line of thinking. I think I just may make “computedProperties” a thing for this new tagged release and see if that solves our woes.
Now, the question is: what is the naming convention of computed property methods:
A) ->computedFoo()
B) ->getFooAttribute()
C) ->getFooProperty()
Here are the pros to each of them:
A) Feels the most intuitive to me. Only one extra word. Is the shortest.
B) Leverages the existing knowledge of Eloquent Model accessors. I like not adding extra API to remember.
C) “getXAttribute” is kinda incorrect, because Livewire calls them “properties” not “attributes” like an Eloquent model. So this would leverage the pattern of Eloquent, but adapt it to a Livewire context.
Here’s my order or preference currently: A, C, B
Thoughts anyone? (on the decision to go with this as well.)
I personally prefer A or C. I like C better myself because again, it has the “Laravel” feel of an accessor, but A is very similar to the lifecycle hooks convention of updatedFoo() so should be natural for people who are comfortable with Livewire. I like either.
Makes sense.
The next problem:
It would be great if computed properties were automatically available to the Blade view as regular variables (like public properties are):
<div>
{{ $post }} // is evaluated from "->computedFoo()"
</div>
Accomplishing this wouldn’t be all that hard. The problem though, is one of the virtues of these “computed” properties is only being evaluated when they are needed (this way if you don’t use the model on every request, the db query won’t be executed when it’s not needed).
Unfortunately, there is no “with” statement like there is in JavaScript that would magically allow us to resolve undefined variables at run-time (like a magic __get method for global variables).
Which brings me to the next logical API for computed properties:
<div>
{{ $this->post }}
</div>
Access to the $this object from a view.
This seems fine to me, although NOW I’m bummed that the user has to know when to use $this->foo and just $foo based on whether or not the property is a public property OR a computed property.
Which honestly, is making me feel like $this->property should be the idiomatic way to access public properties, and we should forget the whole “automatically pass public props to the blade view”. There’s already been requests for access to $this inside the Blade view to call methods, this would make things unified, and actually would feel quite nice.
I think this is what I’m leaning towards… ugh… thoughts?
Update: I made all these updates we’ve talked about.
You can try everything out on dev-master (and please do to vet it for bugs)
Updates made:
$this->property WILL be the idiomatic way.What do we think? Am I crazy? I finally feel pretty good about all this.
Thanks everyone SOOOO much for the help fleshing this out.
Really interesting and well explained thread everyone!
Caleb, are you proposing removing protected (cached) properties entirely with the addition of the computed properties? I like the current cached property for its ability to hold a modified state in memory (Cache) before later committing the changes to the Database model but while also preventing that data from being passed back and forth to the client.
Wouldn’t the proposed approaches loose that ability (of having a dirty, in progress model instance that does not get passed to the browser in its entirety)?
This does remove cached protected properties and would lose the ability you describe. However, you should still be able to do your own caching if you need it. I believe Caleb was going to work on some documentation outlining some best practices / recommendations for implementing your own cache.
I think the loss of the protected/cached properties is a pretty big loss. After thinking about this a bit more however, I was also wondering if the new approaches solve the issue of the counters reverting to 1&1 on the back button. From my understanding, that will not be solved.
On the other hand the counters going to 2&3 on thew subsequent increment seems correct from a certain point of view. The public one goes to 2 because 2 was not saved to any persistent storage previously. The protected goes to 3 (which is correct in terms of number of times it was incremented) because its actual values was persisted in the cache.
So I would respectfully introduce the theory that perhaps this not an actual bug and just an inconvenience caused by the back button scenario. But unless the back button scenario (which is already an edge case) could be resolved, maybe this is not an issue.
Again, don’t mean to stir the pot, just wanted to share my point of view. Curious to hear what you guys think…
I wanted to test version 0.5.0 out on my app, but after changing my composer.json to
“livewire/livewire”: “dev-master@dev” or “livewire/livewire”: “^0.5.0”
I get an error:
Illuminate\Contracts\Container\BindingResolutionException : Target class [blade.compiler] does not exist.
at /app/vendor/laravel/framework/src/Illuminate/Container/Container.php:805
What am I doing wrong?
Hmm, in another app (yes, I’m using Livewire in three different apps already) I could update to 0.5.0 without errors. Strange…
Hey Erik, please submit an issue on the GitHub repo for this. I’d lack to track them there.
@csti, I love the input - it’s not pot stirring.
Good points here. I’ve been thinking about it more over the weekend, and although I agree protected properties is a big loss, I really think the project is better off. It does suck to make Livewire less user-friendly and “invisible”, but I will sleep more soundly at night knowing it’s not juggling 2 completely different state paradigms. I also just have the feeling that we will keep playing weird-edge-case whack-a-mole because of it.
Again, tough one, but I’m feeling more sure of myself on this as I sit with it more.
Ok, the saga continues. Dries is suggesting we rethink this decision and add support for Eloquent models using the “SerializesModels” trait from Queueables.
Here’s the thread on GH: https://github.com/livewire/livewire/issues/488
Input appreciated!
Ok, some decisions have been made.
You can now set public properties as models or model collections