Components should play nicely with Rails caching mechanisms

[ozzyaaron]

Feature request

Components should provide a digest to Rails views, or be able to use Rails explicit caching directive comments or be able to provide a cache_key.

Essentially we should be able to invalidate a cache based on whether the Component class or template has changed.

Motivation

I'm just building on some previous reports primarily around where templates live and the filenames of the template/s. My concerns come down to integrating with Rails caching basics.

The issue that we run into is basically summed up by

## In any view/partial
cache[object, another_object] do
  render(MyComponent.new(attribute: value))
end

In this case if the template for MyComponent is changed the cache will not be invalidated. I tried to add MyComponent to the array used by cache and provide a self.cache_key method that used ActionView::Digestor but without having some further mechanism to invalidate the digestor's cache for the template/s when the template changes this didn't work.

Rails allows for explicit directives to be used (https://guides.rubyonrails.org/caching_with_rails.html#explicit-dependencies) but by default this will only look inside app/views. You can add a view path append_view_path("app/components") but the digestor is expecting a partial and so a template filename would have to be prefixed with underscore.

The way we are currently working around this is to have an ApplicationComponent that our components inherit from and where we override ActionView::Component::Base.matching_views_in_source_location so that templates are now found in app/views/component_templates and where the files are prefixed with underscore.

  def self.matching_views_in_source_location
    return [] unless source_location

    *path, file_name = source_location.chomp(File.extname(source_location)).split("/")
    path.pop # remove components
    path.concat(["views", "component_templates"])
    path_to_template = path.push("_#{ file_name }").join("/")

    Dir["#{ path_to_template }.*{#{ActionView::Template.template_handler_extensions.join(',')}}"]
  end

Then in the above example where cache invalidation does not work via template digest changes we can do

## In any view/partial
# Template Dependency: component_templates/my_component %>
cache[object, another_object] do
  render(MyComponent, attribute: value)
end

I think if AV::C is going to be core or integrate with Rails it would need a way to expose template digests nicely, or provide cache_key probably at the class level. If not then as a last resort we would need a way to use explicit directives in templates that use ActionView::Components internally.

I'd also be all 👂 to people who have solved this already or if I've missed something about the features ActionView::Component provides!

[pinzonjulian]

Happy to see another report on caching! Issue 42 also opened up this question in the early days and is marked with the V3 tag.

I'll start doing some research on my end based on your research @ozzyaaron and try to see if I can take a stab at it 🤘🏽

@joelhawksley , anything I'd need to do or know before starting? All help is welcome!

[joelhawksley]

@pinzonjulian I don't know if there's much I can add here, as my experience with caching is fairly minimal. Anything we can do to improve our compatibility with Rails gets a big 👍 from me 😄

[dkniffin]

@pinzonjulian Not sure if you're still researching this, but I was interested and did some digging on my own. Here's what I found:

• Caching starts with the cache method (or it's variants, cache_if, cache_unless, etc)
• From there, a couple steps down, there's a call to Digestor.digest, passing in a lookup_context
• From what I can tell, lookup_context is used to look up where a given view file is, given a string. So if it's given "posts/post", it might resolve that to app/views/posts/_post.html.erb. We can append paths to that lookup context via eg append_view_path
• Stepping back up and down into that Digestor.digest call, we end up here. From what I can tell, this class's job is to build up a dependency tree and convert it to a digest key. I think the only important part here (for us) is the call to DependencyTracker.find_dependencies, which lands us here
• A few more steps down and we end up here. This is pretty well documented by the "Template digest" docs here. "Implicit dependencies" is render_dependencies and "Explicit dependencies" is explicit_dependencies. I think we're interested in the render_dependencies, though there might be a workaround worth exploring with explicit_dependencies maybe.
• render_dependencies is interesting. It splits the template file's source code based on the render keyword, then calls add_dependencies, which tries to determine if the method is being called with a string (eg "posts/post") or a variable (eg @post). I think this is the part we'd want to behave differently. We'd want that to support a ViewComponent.new somehow.

So, in summary, I think there's two things we'd need to change to get caching working for ViewComponent:

1. I think we need to add our ViewComponent files (at least the template, but ideally also the ruby class file, though I'm not sure if Digestor.digest is set up to handle non-template files) to lookup_context, maybe via append_view_path.
2. We need to update DependencyTracker to handle a ViewComponent instance as the argument to render.

I'm not sure how much of that I misunderstood, but hopefully it helps! @joelhawksley perhaps you know of someone at Github with more experience in this area that could spend some time looking over this and make a recommendation? I'd be happy to make a PR to ViewComponent and/or Rails, but I want to make sure I'm on the right track before I dive in over my head.

[pinzonjulian]

Hey @dkniffin ! I've been working on and off on this and I've been documenting my progress here:

https://www.notion.so/View-Component-07d8bef1ed3342498b5805cc3166a82a

It's a little messy because I wasn't planning on sharing those but I've had very little time lately to put more time into it. Take a look at the section that says "The whole journey" for a breakdown of all the path Rails takes to cache something.

I've loved digging into this but it's been pretty rough without any guidance so I'm glad you're chiming in!

I'd like to bring my findings into Github somehow so we can build upon it together but not sure how. Any ideas @joelhawksley ?

[dkniffin]

@pinzonjulian that's awesome! We both followed through it all and came to nearly the same conclusion. The bit that I think you missed was the part where Digestor.digest builds up a dependency tree. I think the dependencies that's passed in there is a red herring and the actual important dependency building is in the call to tree

[pinzonjulian]

Yeah I definitely remember the dependency tree being built but that's where I started slipping a little. I couldn't understand how to register a new location for Rails to look for dependencies. I'm curious to see if fixing that would unblock all of the possible ways for caching (there's a comprehensive list in my notion page).

I'd really like to help and keep learning about this so count me in in any further development if possible!

[joelhawksley]

@dkniffin @pinzonjulian thank you for all of the time you've spent researching this issue! Perhaps one of you could provide a PR with a failing test for us to work against? I'd love to set up some time to pair on a solution.

[dkniffin]

@joelhawksley I would be glad to do that, but I'm not quite sure how that test would look. This is the template I'd want to test:

<%= cache do %>
  <%= render MyComponent.new %>
<% end %>

and the result I'd be testing for is that if the code for MyComponent.new changes, the cache gets invalidated. But I'm not sure how to do that. Specifically, I guess I don't know how I'd change out the code for MyComponent (the template and/or the class).

Edit: I found this test in the rails codebase that's kinda similar, but again, what I'd need to do is test that a change to the component invalidates the cache. I think that test I linked is a little different.

[dkniffin]

@jhawthorn I hope you'll forgive me for summoning you to a random repo you haven't contributed to before, but I saw you made a somewhat recent change to ActionView::DependencyTracker, so I'm hoping you might have a better idea of how this works and/or how to make a test for this and/or how we might get this working. 😅

My team and I would really like to use this library, but I'm afraid that this lack of caching support may be a blocker for us

[dkniffin]

@joelhawksley Any progress on this?

[joelhawksley]

@dkniffin I haven't had the time, sadly. Would you be willing to write a PR with a failing test? Perhaps we could pair together on it 😄 Email me: joelhawksley@github.com

[dkniffin]

@joelhawksley I definitely empathize :/ I haven't had much time to dedicate to this either. I would love to create a PR with a failing test, but I honestly don't know what that failing test would look like. I can't wrap my head around a proper way to test that changing the contents of a file busts the Rails cache, unless I use File.open or something, but that seems brittle. But maybe it'll work? 🤷‍♂️

However, a team member and I have thought a little about how this feature could be implemented. As stated above, I think there's two things that need to change:

1. We need to ensure that Rails knows to look for template files in the app/components directory. (I still haven't thought through this a lot, but I think this should be an easy problem to solve)
2. We need to update DependencyTracker to handle a ViewComponent instance as the argument to render. I think that means updating ERBTracker#add_dependencies to look for a regex like /.*Component.new/.

I hope to find some time at some point to test the idea in 2 out. I think that would make more sense as a PR to Rails, but I'm not sure how to have that regex not be dependent upon how ViewComponent classes are named. Maybe I could make it generically "look for any instances of a .new call" or something. 🤷‍♂️ If anyone has thoughts on that, I'm all ears!

[joelhawksley]

@dkniffin we have tests that change files, see https://github.com/github/view_component/blob/master/test/view_component/integration_test.rb#L48 for an example.

Perhaps you could build off that?

[dkniffin]

@joelhawksley Oh, awesome! I didn't know there was something like that in the codebase already. Yes, that should make writing that test a bit easier. I'll see if I can take a stab at it sometime this week.

[dkniffin]

@joelhawksley Alright, I've created that PR: #476

Now for the solution. I don't think there's a way to accomplish #2 above without some changes to Rails, specifically in ERBTracker. I have a rough idea of what needs to change there, so I'll take a stab at it. Hopefully the Rails team is receptive to the change.

Edit: I dug into the code with a debugger a bit and tried to trace what was going on, but had some difficulties. For some reason, in that ERBTracker, I was receiving a subset of the code I thought I would, and it didn't even include the render line for my component class. Perhaps I need to solve # 1 above first, idk. I'll see if I can dig in again some more tomorrow.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dkniffin]

Bump

[dkniffin]

@joelhawksley A few of my teammates suggested a possible "workaround" to the problem here. We could add a new ViewComponent#md5 method that returns a hash of the contents of the files for the component, so that if the component changes in any way, the hash does to. Then, that method could be used in the cache call from Rails.

For example:

cache([other_cache_keys, MyComponent.md5]) do
  render(MyComponent.new(attribute: value))
end

It's still not perfect because users will have to remember to do that, and if there's a cache call at the top of many layers of abstraction, it would have to be aware of the component call deep below to include it, but it's a start.

What do you think?

[ozzyaaron]

I'm pretty sure #cache_key is called by the cache call on each element in the array. We attempted to create a cache_key method on the Component (functionally the same as having to call md5) but the issue is that you either need to make an opinionated choice like do a full run of templates every time in development OR you need to hook into the existing Rails mechanisms which are or are not watching for file changes in the locations or ways that ViewComponent is expecting.

I think it would be nicer if ViewComponent tracked with the Rails configuration including things like prepended view paths and whether classes are being cached or not.

Further what we're seeing in the latest release (2.20.0) the way templates are found is becoming harder to interact with (we used to override .matching_views_in_source_location in our base ApplicationComponent < ViewComponent::Base) and is becoming less configurable. We moved our templates into a more standard Rails location so we could use explicit cache hinting but we're now locked on a previous version until we can figure out how to use these hints again.

I think it would be great if in the short term the class itself (derived from ViewComponent::Base) had a simple method to return its template location, or if ViewComponent could have its base template location changed.

[joelhawksley]

I think it would be nicer if ViewComponent tracked with the Rails configuration including things like prepended view paths and whether classes are being cached or not.

Agreed. We've already moved in that direction with #509.

I think it would be great if in the short term the class itself (derived from ViewComponent::Base) had a simple method to return its template location [...]

I'd be 👍🏻 to this change, and have started work on implementing Base.template_locations. @ozzyaaron what would you be willing to suggest what the return value of this method should look like?

We moved our templates into a more standard Rails location so we could use explicit cache hinting

I'm sad that this was necessary for you, and hope we can improve ViewComponent so it no longer is ❤️

[ozzyaaron]

Hi @joelhawksley thanks for the questions! We're getting a lot out of using ViewComponent thus far and its free, so please don't take my opinions negatively, we're thankful for what we get 😄 🦀

Agreed. We've already moved in that direction with #509.

Thanks! IMO a great move :)

I have to do a bit more testing but I think when I was looking at 2.20.0 the issue I saw was that matching_views_in_source_location was no longer being called on our own superclass but on ViewComponent::Base so 2.21.0 may fix that for us. I need to check :)

EDIT: upgrading to 2.21.0 didn't fix it for us. I'll probably be able to take more of a look at it tomorrow but it doesn't appear to call the component class's matching_views_in_source_location method anymore and just calls it on the ViewComponent::Compiler class.

On the ability to have a method template_locations I think it may need a bit more consideration to make it easier to people. To me there are 2 concerns that we have found to be a bit difficult to override hence we had completely re-written matching_views_in_source_location for our own purposes including being able to use inherited templates.

This is just my opinion based on our use so far. I personally think there are 2 concerns

• base path (where the templates can be found)
• template file name or pattern

I think you could probably get away with just allowing people to define both together (e.g. template_location which might be app/components/component and then it would find the right variant and postfix the extension like html.js) but having the two concepts be separated may make it more customisable to users and probably allow easier hooks to Rails wrt prepend base_path to view paths, and adding base_path to the ActionView::LookupupContext and so on which should aid in being able to monitor that for updating template digests as required. For use we'd probably just do def template_path; Rails.root.join("app/views/component_templates").to_s; end

The reason I separately suggest a template filename is for the concern of users and formats/variants. As a user I'd far prefer to just deal with a template name (perhaps as simple as a template_name method on Base that can be overridden by inheritors) and then ViewComponent would use that to find the correct variant. Largely I don't see people using something like render "action_name.js" so I think being able to have a def template_name; #{ self.class.underscore }; end which then ViewComponent uses later with the appropriate format would be cool! For us we may just do def template_name; "_#{ super }"; end

For most users I'd imagine they'd never notice and not have to change anything with changes like the above as ViewComponent::Base can easily provide the same defaults it has assumed up until now. For us we'd just override the template_name and base_path in our ApplicationComponent superclass.

I'm sad that this was necessary for you, and hope we can improve ViewComponent so it no longer is ❤️

Hey man, don't worry! We used a couple component frameworks previously and they all had major flaws even for our meager use cases. ViewComponent is one we have jumped into feet first though and have only had to override 1 method thus far. I'd love to be able to help out but have a couple other outside-work projects I'm already helping on. I do think if ViewComponent were able to get the template path exposed and then add an effective watcher to the Rails cache digestor mechanism it would sort out most of our current concerns. Even with the small issues, ViewComponent is making our code easier and more robust for sure!

Thanks to all the contributors! 💙

[joelhawksley]

@ozzyaaron thanks for your detailed feedback!

Can you provide more context on why you're choosing to locate templates in a separate directory from the component objects? If it's just for the sake of caching, I feel like we should pursue supporting caching properly with the existing template organization structure.

FWIW, we did have a version of ViewComponent a while back that used templates from app/views, but it made it easy to break the encapsulation ViewComponent provides, as the templates were then accessible in other Rails views outside the context of their associated component.

[ozzyaaron]

@joelhawksley you're absolutely right that we have just chosen to change the location and template prefix to work with Rails caching. I think if we were able to have ViewComponent support caching normally through calls like cache(current_user.locale, object, Component) that would be ideal for us and probably for everybody :) I am certain we could then just revert the modifications we've made to template finding.

In some places I've been there were some use cases for sharing templates outside of the Rails or server-side framework. With a pretty small bridge/adaptor we've rendered template via multiple mechanisms. For example writing templates that would be rendered by Node and Ruby systems - especially when distributing to multiple targets like mobile, desktop and the web. I'm not suggesting that as a use case for ViewComponent just as an interesting outcome I've seen really investing in component templates previously.

[dkniffin]

Has there been any progress made on this in the past year? I see it's been marked for the V3 release

[joelhawksley]

@dkniffin not to my knowledge. We don't do much view caching at GitHub, so we aren't able to prioritize this work internally. We tagged this issue with the v3 label as we view it as a blocker on the v3 release.

[dkniffin]

I'm still keeping an eye on this thread, hoping for something to get added 🤞

I wanted to mention that #980 implemented a POC of how this could work, since it's not linked in this thread.

[erikaxel]

@joelhawksley Is there anything you can do to unblock this on the Rails side of things? You seem to have been able to guide the initial commit to Rails 6.1 that enabled ViewComponent in the first place, so maybe you have some tricks? From an outside view it seems like the PR needed at the Rails side is stalled and it is a bit unclear to me why: rails/rails#42850

[joelhawksley]

@erikaxel I'm sorry to say that I don't think I have much sway here. Caching support for ViewComponents is likely a fairly significant effort that we (GitHub) don't currently have the resources to take on.

I'm guessing @Matt-Yorkley would be keen for someone to step up and try to land that PR, for what it's worth. Perhaps you and @dkniffin could resurrect it? I'd be happy to provide pairing support as needed ❤️

[Matt-Yorkley]

I dropped it because there seemed to be zero interest on the Rails Core side and a strong reluctance to allow the proposed changes to some of the relevant ActionView classes that handle creating/mapping the digests for each fragment. I don't think it's feasible to do it without some of those changes.

[dkniffin]

@joelhawksley I'm still unclear on why the Rails PR (rails/rails#42850) won't be accepted. Is there more work required to make it acceptable in Rails Core? imo, a complete re-write of the rails caching system would be great, but short of that happening, it seems like the changes @Matt-Yorkley made are a reasonable short-term solution.

[joelhawksley]

@dkniffin I don't have any context beyond what you can read on the PR 💔 I wish I had a better answer for you 😞

[marckohlbrugge]

We don't do much view caching at GitHub, […]

@joelhawksley Forgive me if this is slightly off-topic, but would you mind elaborating on why GitHub doesn't need view caching? How do you ensure a speedy response without the popular Russian-doll caching approach?

Perhaps there are useful insights here for other View Components users to side-step this whole caching issue.

[joelhawksley]

@marckohlbrugge we cache at the request level for logged-out traffic. For logged-in traffic, we cache RPC calls vs. view rendering as view rendering is generally not a bottleneck for us but RPC calls are.

Much of GitHub's UI changes based on a user's permissions so it is difficult to get a high cache hit rate. We do cache rendered markdown though, as it is more or less viewer-agnostic. It's also tricky to do view caching in combination with feature flags that modify the DOM, which we do quite a bit of.

[BlakeWilliams]

I think #1650 might make components and caching play well together. It would be nice to have a test that can be run against that PR, but I likely won't tackle that any time soon. If anyone is interested in writing a test for that and opening a PR, please do!

[joelhawksley]

@Matt-Yorkley would you be up for helping @BlakeWilliams validate if #1650 improves caching compatibility?

[Matt-Yorkley]

It looks like a great PR, but I don't think it'll directly resolve the issues with fragment caching...

[jcoyne]

Yes, #1650 definitely doesn't improve the caching problem.

ActionView::DependencyTracker::ERBTracker#render_dependencies

still includes "news/new" this is from scanning MyComponent.new and deciding (via regex) that new was the dynamic part. Then it plurarlizes it to make "news" and prepends that to make news/new. We need a way for ActionView::DependencyTracker.find_dependencies to recognize that MyComponent.new is a renderable, not some sort of ERB template.

[reeganviljoen]

@joelhawksley I would be interested in taking this on
here are some of my thoughts that I plan to implement

I plan on adding a cache_on macro to each component that accepts a method name/proc that uses these attributes to build a cache digest

I feel this is the best way as it allows the component to control its own caching dependant on its own local state wich I feel is a better fit than caching on external global state that the component has no context on, it will also be easier to debug by just trowing a binding.irb in the cache methods

I would highly appreciate your thoughts