Add first class component cache #2126
Conversation
reeganviljoen
changed the title
reeganviljoen
changed the title
reeganviljoen
force-pushed
the
rv_add_component_caching
branch
from
7544079
to
7533a9c
Compare
|
@joelhawksley @Spone @camertron I would appreciate any thoughts on this. |
There was a problem hiding this comment.
@reeganviljoen THANK YOU for taking a crack at building something functional to address the long-overdue need for caching in ViewComponent.
To be honest with you, I have not used caching in Rails nearly enough to feel like I can properly assess this PR in terms of how it would integrate with the existing mental model of caching in Rails. I'm happy to learn that context, but I won't have the space to do so for at least a couple of weeks.
@Spone @boardfish have y'all done much with Rails' caching? @reeganviljoen if you know of anyone else in the community who would be a good reviewer here, I'd be happy to have them.
@joelhawksley I really aprreciate the honesty, I will see if I can find anyone with cache experience to take a crack at reviewing this ❤️ |
|
@joelhawksley Afraid it's not something I've really interacted with directly until now, sorry! I'd be coming from a similar place. Great to see @reeganviljoen working at it, though ✨ |
Co-authored-by: Emil Kampp <[email protected]>
Co-authored-by: Emil Kampp <[email protected]>
lib/view_component/base.rb
Outdated
| @@ -34,6 +35,7 @@ def config | |||
| include ViewComponent::Slotable | |||
| include ViewComponent::Translatable | |||
| include ViewComponent::WithContentHelper | |||
| include ViewComponent::Cacheable | |||
There was a problem hiding this comment.
Suggestion, nitpick and nonblocking: Can we slide this in alphabetically above? I'd think it'd be ~ line 32
| __vc_cache_dependencies.push(*args) | ||
| end | ||
|
|
||
| def inherited(child) |
There was a problem hiding this comment.
We took this for a spin in a few cases of our cached partials and view components that we are familiar with and largely it works for the base cases. This is working to correctly bust a cached VC when it changes or things in the render path 'above' (partials or VCs) it change.
What we aren't seeing is when a VC renders another VC or partial as a child that downstream changes of the cached VC pick up changes and handle it. I'm going to fork your branch and offer some test cases based on an approach we use in the vc fragment caching gem (what we're currently solving this with)
Effectively we want some way for a child partial or VC change (in either the .rb or template) to bust the cached parent.
There was a problem hiding this comment.
@JWShuff like touch: true in rails ?
There was a problem hiding this comment.
also big thanks for testing it, I can look at adding the touch true thing later this week
test/sandbox/test/rendering_test.rb
Outdated
There was a problem hiding this comment.
@reeganviljoen I am not a git-wizard, much to my eternal shame, but I set up a fork and adjusted the spec approach here to use the integration examples/controllers to assert the behavior. Feel free to take, leave, or otherwise.
There's a wider challenge around partial/template digesting, and on a quiet day I'll port our spec suite over to the VC style and get it implemented so we have all the permutations we know of that need to cache appropriately.
…omponent#2164) Hello! We have been using ViewComponent for a while now for a growing number of our components, so thought I should add us to the list of orgs. Cheers Steve
|
@JWShuff I investigated a few of your suggestions, added a few, thanks, and others I am struggling with |
|
@joelhawksley with a live use case tested how do you feel about this now |
closes #234
What are you trying to accomplish?
Add a caching mechanism that is true to the view_component ethos namely
What approach did you choose and why?
I chose to add a caching macro that takes in method names that are to be used to compile a cache digest, that digest is then used to cache the contents of the call method
Anything you want to highlight for special attention from reviewers?
This is a first effort at getting first class cache for VIewComponent if this is accepted I plan to add more featires like support for procs, if options, etc