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

Cache Octicon SVGs for same icon/options combination #491

Merged
merged 3 commits into from
Sep 2, 2020
Merged
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
4 changes: 4 additions & 0 deletions docs/content/packages/rails.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,7 @@ This rails helper lets you easily include svg octicons in your rails apps.
```

We recommend including the CSS in the [`@primer/octicons`](/packages/javascript) npm module. You can also npm install that package and include `build/build.css` in your styles.

## Caching

`octicons_helper` caches repeated calls to `octicon` based on the name of the icon and the provided `options` hash. Avoid arbitrary/high cardinality `options` (such as an `id` with a unique record identifier), as they can cause a memory leak.
16 changes: 13 additions & 3 deletions lib/octicons_helper/lib/octicons_helper/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,23 @@
require "action_view"

module OcticonsHelper

include ActionView::Helpers::TagHelper

mattr_accessor :octicons_helper_cache, default: {}

def octicon(symbol, options = {})
return "" if symbol.nil?

icon = Octicons::Octicon.new(symbol, options)
content_tag(:svg, icon.path.html_safe, icon.options) # rubocop:disable Rails/OutputSafety
cache_key = [symbol, options].to_s

if octicons_helper_cache[cache_key]
octicons_helper_cache[cache_key]
else
icon = Octicons::Octicon.new(symbol, options)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should include any of the raise conditions here when the symbol doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonrohan Good idea! I think we should look into it in another PR.


tag = content_tag(:svg, icon.path.html_safe, icon.options).freeze # rubocop:disable Rails/OutputSafety
octicons_helper_cache[cache_key] = tag
tag
end
end
end
22 changes: 22 additions & 0 deletions lib/octicons_helper/test/octicons_helper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,27 @@
it "adds html attributes to output" do
assert_match /foo="bar"/, octicon("alert", foo: "bar")
end

it "caches SVGs for two calls with the same arguments" do
OcticonsHelper.octicons_helper_cache = {}

mock = Minitest::Mock.new
def mock.path
@@call_count ||= 0
@@call_count += 1

raise "Octicon library called twice" if @@call_count > 1

"foo"
end
def mock.options; end

Octicons::Octicon.stub :new, mock do
octicon("alert")
octicon("alert")
end

OcticonsHelper.octicons_helper_cache = {}
end
end
end