Skip to content
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

WIP caching module #39

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/fortitude/rails/renderer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module Rails
class Renderer
class << self
# TODO: Refactor this and render :widget => ... support into one method somewhere.
def render(widget_class, template_handler, local_assigns, is_partial, &block)
def render(widget_class, template_handler, local_assigns, is_partial, options = {}, &block)
if ::Fortitude::Erector.is_erector_widget_class?(widget_class)
return ::Erector::Rails.render(widget_class, template_handler, local_assigns, is_partial, &block)
end
Expand All @@ -25,6 +25,11 @@ def render(widget_class, template_handler, local_assigns, is_partial, &block)
end

widget = widget_class.new(needed_assigns)

if options[:pathname]
widget.instance_variable_set(:@virtual_path, options[:pathname])
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order for ActionView::Helpers::CacheHelper to work properly, the @virtual_path instance variable must be set: https://github.com/rails/rails/blob/215f86c3483cf300b2ccd6d5d8b97f9c9bf547f5/actionview/lib/action_view/helpers/cache_helper.rb#L230

See discussion in template_handler.rb below:

end

template_handler.with_output_buffer do
rendering_context = template_handler.controller.fortitude_rendering_context_for(template_handler, block)

Expand Down
4 changes: 3 additions & 1 deletion lib/fortitude/rails/template_handler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ def call(template, &block)

is_partial = !! File.basename(template.identifier) =~ /^_/

pathname = "#{template.identifier =~ %r(views/(.*)) && $1}"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to pass the @virtual_path to the widget as per the comment above.

However, I'm not sure this code will work with views located outside of the normal view path of $RAILS_ROOT/app/views. What about views in other view paths, or in a gem?

This code was adopted from erector-rails4 (actually, I just noticed that I wrote it... woah...): https://github.com/ajb/erector-rails4/blob/d2fb36cd2f15d075b1f7c371b0c0978775b52908/lib/erector/rails/template_handler.rb#L6

...but it should hardly be considered bomb-proof.


<<-SRC
Fortitude::Rails::Renderer.render(#{widget_class.name}, self, local_assigns, #{is_partial.inspect}) { |*args| yield *args }
Fortitude::Rails::Renderer.render(#{widget_class.name}, self, local_assigns, #{is_partial.inspect}, pathname: "#{pathname}") { |*args| yield *args }
SRC
end

Expand Down
3 changes: 3 additions & 0 deletions lib/fortitude/widget.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ class Widget
if defined?(::Rails)
require 'fortitude/rails/widget_methods'
include Fortitude::Rails::WidgetMethods

require 'fortitude/widget/caching'
include Fortitude::Widget::Caching
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching depends on Rails, so it's required down here.

else
require 'fortitude/widget/non_rails_widget_methods'
include Fortitude::Widget::NonRailsWidgetMethods
Expand Down
38 changes: 38 additions & 0 deletions lib/fortitude/widget/caching.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
module Fortitude
class Widget
module Caching
extend ActiveSupport::Concern
include ActionView::Helpers::CacheHelper

module ClassMethods
# PUBLIC API
def cacheable(opts = {})
if extra_assigns == :use
extra_assigns :ignore
end
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to use extra assigns when calculating the cache key, since this would make the cache key be too variable. (@marked_for_same_origin_verification was getting set in my Rails app, for example.) However, this means that the developer cannot use extra assigns in their widget. This is a bit ugly... maybe we could raise an error if the developer tries to access something in extra_assigns instead?


static_keys = Array(opts.fetch(:key, []))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows for doing something like:

class MyWidget
  cacheable key: 'v2'
end

Although heck, if the template is going to be digested, then we could just as well remove this code and instruct this usage:

class MyWidget
  cacheable # v2
end

I guess the only reason this code would be necessary is when using the :skip_digest option.

options = opts.fetch(:options, {})

define_method(:cache_contents) do |&block|
cache calculate_cache_dependencies(assigns, static_keys), options do
block.call
end
end

around_content :cache_contents
end
end

private

def calculate_cache_dependencies(assigns, static_keys)
(
assigns.to_a.sort_by(&:first).flatten +
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is me trying to figure out the best way to generate a cache key from the assigns hash. By coercing to an array (instead of just using the #values), we avoid key collisions like this:

{ foo: @bar, baz: nil }

vs

{ foo: nil, baz: @bar }

To be honest, I'm not really sure why I'm sorting the array. I guess I'm not sure if I'll receive assigns in the same order every time, and I want to maximize the potential of a cache hit.

static_keys +
[widget_locale]
)
end
end
end
end
10 changes: 10 additions & 0 deletions spec/rails/cacheable_method_system_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
describe "Fortitude cacheable method behavior in Rails", :type => :rails do
uses_rails_with_template :cacheable_method_system_spec

it "caches properly" do
expect_match("localization?locale=en", /hello is: hello 1/)
expect_match("localization?locale=en", /hello is: hello 1/)
expect_match("localization?locale=fr", /hello is: bonjour 2/)
expect_match("localization?locale=fr", /hello is: bonjour 2/)
end
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test kinda sucks. I'd like to test some of this stuff with a system spec (i.e. a non-integration test), but those don't appear to require Rails.

end
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class CacheableMethodSystemSpecController < ApplicationController
before_filter :set_locale

def set_locale
I18n.locale = params[:locale] if params[:locale]
end

def localization
# nothing here
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class Views::CacheableMethodSystemSpec::Localization < Fortitude::Widgets::Html5
cacheable

def content
text "hello is: #{t('.hello')} #{times_called}"
end

def times_called
@@times_called ||= 0
@@times_called += 1
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
en:
cacheable_method_system_spec:
localization:
hello: "hello"
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
fr:
cacheable_method_system_spec:
localization:
hello: "bonjour"