-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Widget caching #54
base: master
Are you sure you want to change the base?
Widget caching #54
Conversation
@ajb — this is great! I’ve been spending a lot of time thinking about this, and I’m currently leaning towards the idea of making caching a separate One question I have for you: the |
Yeah, you've got it. Rails looks at the |
OK, I just went spelunking down quite the rabbit-hole. I have a question for you, and then a broader discussion: Question: What specific case was failing for you that caused you to add the Discussion: I just discovered a giant piece of madness in Rails I didn’t even realize existed — this code. In short, it tries to use regular expressions on ERb template files to build a dependency tree for a view. The goal is to add the hashes of a view template, and every template that view template calls, recursively, to the cache digest used in the There are a couple of problems with this. Generic to Rails, this kind of “dependency tracking” is very far from perfect, since it’s using regular expressions to parse code. It’ll catch common patterns like changing a partial you call from your template, but it definitely won’t catch things like calls to helpers that are defined elsewhere, and so on. Specific to Fortitude, in order to have this sort of dependency tracking work properly, someone would need to write code that tried to parse Fortitude widgets to determine dependencies. This is probably a significantly more difficult task than it is for ERb. (Ruby syntax is more flexible than ERb, there are more ways to invoke other widgets than rendering partials, Fortitude code is going to be more highly factored — which is part of the whole point — and so on.) I think I read something about Ruby exposing its own AST to other Ruby code more easily in recent versions, so maybe that could be used? — but it still seems like quite a task. Currently, I’m disinclined to try to tackle that whole bag of worms. I think the result would be something that would “mostly work” — which, personally, is something I kind of hate in software, because it’d work often enough you’d tend to forget about it, but, when it failed, the kinds of bugs it causes can be really hard to track down, and you wouldn’t stop to think about this as a potential cause for quite some time. The ROI just doesn’t seem to be there. It’s much cleaner just to tell people “don’t turn on caching in development, or, if you do, you have to clear the cache manually when you change templates; in production, make sure you bust the cache on every deploy”. That’s honestly something you really need to do to get 100% correct results using ERb fragment caching anyway. Coming back to your original proposal — what I’d love to do is to sort out any remaining |
I agree with you about dependency tracking, and not trying to implement it for Fortitude. I think that would quickly turn into a losing battle for the reasons you mention. I'll look into your |
I'm having a lot of trouble getting my test suite to work properly, as well as getting this PR up-to-date with master, but I'm pretty convinced that the |
Re: test suite — let me know if I can help. I saw the issue you open/closed around Re: There is something slightly tricky that goes on here — it doesn’t change the outcome, but it’s worth understanding. Under ERb, due to all the weird instance-variable copying that happens, Under Fortitude, your widget class is much “cleaner” — it’s a completely separate object, and thus I’m not aware of any place where this actually breaks something, but it’s worth noting in case it makes a difference… |
Gotcha! I suspect then, that the issue is that I'm calling |
Ah, excellent. That does make sense. You shouldn’t actually need to include If you remove that line, does it actually break? If so, how? |
I will have to get back to you on this, once I fix these broken tests :) On Mon, Oct 17, 2016 at 11:51 AM, Andrew Geweke [email protected]
Adam Becker |
OK, I've got my dev environment back to normal, and I just rebased this branch on top of master. As we expected, everything works fine when I remove The only question left is: when a widget is rendered via |
OK, awesome! Glad to hear it. Re: If it were me, I’d simply accept this behavior as fine, and document it, since it’s a pretty edge case — and probably mostly applies to development mode, where view caching is disabled by default anyway. The alternative would be to check for the presence of You probably have a better idea than I do what the right path is, though — you’re using In fact, if you have a few, I’d be curious to know what causes you to use |
With regard to busting the cache, I you're assuming that the cache will usually be manually cleared during a deploy? (Correct me if I'm wrong.) With Rails fragment caching, I believe the recommended behavior is not to do that, since you want to have as warm of a cache as possible. As for My main concern is that we make some mistake and use I think raising an error would be appropriate, and in the case that you're aware of the fact that the template cannot be digested, and want to cache anyway, you can pass I'll take a shot at creating an external gem for this. |
Hmm, I'm getting an undefined method error for the I was expecting this to work: |
Re: your last comment — #43 doesn't actually add the Rails helpers to every view. All it does is change it so that if Fortitude’s The issue you’re running into is one I think you’ve run into in the past. In short, the only place you can use Rails helpers is in an actual view or partial, or something else rendered via a controller action. What you’re doing with Fortitude is equivalent to doing this with ERb: puts ERB.new("Helper is: <%= distance_of_time_in_words_to_now(Time.now - 1_000_000) %>").result(binding) If you do that, even running in a Rails console, you’ll see it fail…for the exact same reasons as it does in Fortitude. Here’s what’s going on: Rails helpers — despite many appearances otherwise — don’t actually exist as global methods all over Rails. Instead, what happens is that when Rails gets ready to render a view, it creates this magic “context” object — an instance of an anonymous subclass of Critically, it’s important to note that there is no such thing as a “global” or “generic” view rendering object, nor could there be. There are at least two reasons for this: (a) exactly which helpers end up in that object depends on the exact controller/action you’re executing; and (b) some helpers require additional context to be set on that object — like Above, you’re trying to render a view without a view rendering object. Under the scenes, Fortitude notices this, and ends up using a new bare Views::Test.new.to_html(Fortitude::RenderingContext.new(delegate_object: my_view_rendering_object)) …and it would all “just work”. This is just like doing in ERb: puts ERB.new("Helper is: <%= distance_of_time_in_words_to_now(Time.now - 1_000_000) %>").result(my_view_rendering_object.send(:binding)) If you really need to render lots of widgets outside the context of a Rails controller/action, it’s probably possible to gin up a fake view rendering object that does a “good enough” job, and pass that in…but it’s never going to be perfect; for example, what would it use for that host/port on the incoming request, since there isn’t an incoming request? To me, the gotchas are too big to make Fortitude hack together one of those itself, since it’d always be a pretty gross hack of some kind. This is very clearly something I need to put into the Fortitude FAQ — I believe jdickey ran into the exact same issues. It’s a subtle and tricky set of issues, and not at all obvious to nearly anybody since it’s almost always covered up by Rails’ internal magic. I hope this makes sense, and outlines why you’re running into that issue. One question — what’s the reason for trying to render using a bare |
Re: caching and The problem of stale views is going to be there, unfortunately, no matter what you do. Even with ERb, helper methods and various unusual ways of invoking partials (and probably other stuff, too) is going to go undetected by that (IMHO very janky) DependencyTracker, and you’re going to have cache-invalidation headaches already. With Fortitude they’re worse, because it can’t track child dependencies at all. Ironically, I’m actually reluctant to worry about However, when you build your gem, this can be completely under your control. All you need to do is add a def cache(*args, &block)
raise "No @virtual_path!" unless shared_variables[:virtual_path]
super(*args, &block)
end …and there you go. Last, crazy crazy crazy idea: I realized that there’s a way of building dependency tracking for caching in Fortitude that’s actually probably at least as good as ERb, if not better, and is a lot less gross. Basically, in order for any Fortitude widget to get rendered, it needs a So, it’d be easy enough to create a subclass of |
Thanks for the patient explanation. (Again?) This certainly makes sense. I'm really embarrassed that after working with Rails for years, there is still magic like this that trips me up time and time again. Here's a v1 of the fortitude-caching gem: https://github.com/dobtco/fortitude-caching. I was already working on this when you posted your last comment, so give me a little while to read it and digest :) |
Whoops, I got confused…I’d explained that bit of madness to other people before in different threads, but not you. My apologies! Take it as evidence that it trips up everyone. I certainly didn’t have any freaking clue that that’s how that stuff worked before I wrote Fortitude, and I’ve been doing Rails since 2008, so…yeah. Some of the guts of Rails is pretty crazy. |
A slightly more updated version of #39