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

Add ThemeAssetsReader which reads assets from a theme #5364

Merged
merged 9 commits into from Sep 20, 2016
2 changes: 1 addition & 1 deletion .rubocop.yml
Expand Up @@ -16,7 +16,7 @@ Lint/UnreachableCode:
Lint/UselessAccessModifier:
Enabled: false
Metrics/AbcSize:
Max: 20
Max: 21
Copy link

Choose a reason for hiding this comment

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

That's cheating! 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

😇

Metrics/ClassLength:
Exclude:
- !ruby/regexp /features\/.*.rb$/
Expand Down
2 changes: 1 addition & 1 deletion features/theme.feature
Expand Up @@ -17,7 +17,7 @@ Feature: Writing themes
Scenario: A theme with SCSS
Given I have a configuration file with "theme" set to "test-theme"
And I have a css directory
And I have a "css/main.scss" page that contains "@import 'style';"
And I have a "css/main.scss" page that contains "@import 'test-theme-black';"
When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
Expand Down
1 change: 1 addition & 0 deletions lib/jekyll.rb
Expand Up @@ -55,6 +55,7 @@ module Jekyll
autoload :PostReader, "jekyll/readers/post_reader"
autoload :PageReader, "jekyll/readers/page_reader"
autoload :StaticFileReader, "jekyll/readers/static_file_reader"
autoload :ThemeAssetsReader, "jekyll/readers/theme_assets_reader"
autoload :LogAdapter, "jekyll/log_adapter"
autoload :Page, "jekyll/page"
autoload :PluginManager, "jekyll/plugin_manager"
Expand Down
6 changes: 5 additions & 1 deletion lib/jekyll/page.rb
Expand Up @@ -40,7 +40,11 @@ def initialize(site, base, dir, name)
@base = base
@dir = dir
@name = name
@path = site.in_source_dir(base, dir, name)
@path = if site.in_theme_dir(base) == base # we're in a theme
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain a bit more as to what's going on here?

Copy link

@ghost ghost Sep 19, 2016

Choose a reason for hiding this comment

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

@benbalter AFAIK, it essentially makes sure that we're normalising into the theme directory, rather than into the source directory, otherwise you might get /home/user/work/home/user/.gems/[...]/theme.html instead of /home/user/.gems/[...]/theme.html.

@parkr Have you tried this out with various symbolic links? I'm worried it might be a bit weird, depending on how base is set (I haven't checked, myself)

Copy link
Member Author

Choose a reason for hiding this comment

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

@benbalter When you include assets/styles.scss with YAML front matter, it's a Jekyll::Page. To ensure it's read properly, we need to make sure we're looking in the theme's directory rather than your site's directory. This if says "if this Page is inside our theme directory then ensure the @path gets set properly."

site.in_theme_dir(base, dir, name)
else
site.in_source_dir(base, dir, name)
end

process(name)
read_yaml(File.join(base, dir), name)
Expand Down
1 change: 1 addition & 0 deletions lib/jekyll/reader.rb
Expand Up @@ -18,6 +18,7 @@ def read
sort_files!
@site.data = DataReader.new(site).read(site.config["data_dir"])
CollectionReader.new(site).read
ThemeAssetsReader.new(site).read
end

# Sorts posts, pages, and static files.
Expand Down
47 changes: 47 additions & 0 deletions lib/jekyll/readers/theme_assets_reader.rb
@@ -0,0 +1,47 @@
module Jekyll
class ThemeAssetsReader
attr_reader :site
def initialize(site)
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is copied from another reader. I don't have a personal preference and we don't have it enforced in our Rubocop config.

@site = site
end

def read
return unless site.theme && site.theme.assets_path

Find.find(site.theme.assets_path) do |path|
next if File.directory?(path)
if File.symlink?(path)
Jekyll.logger.warn "Theme reader:", "Ignored symlinked asset: #{path}"
else
read_theme_asset(path)
end
end
end

private
def read_theme_asset(path)
base = site.theme.root
dir = File.dirname(path.sub("#{site.theme.root}/", ""))
Copy link

Choose a reason for hiding this comment

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

We do this same thing at least 2 different ways elsewhere, in the Jekyll::Layout we use the equivalent of path.sub(site.theme.root, "") and in the Jekyll::Document we use Pathutil and there are probably more ways we're doing this. 😖

Copy link
Member Author

Choose a reason for hiding this comment

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

Would be nice to have a "relative_from" method or something but it depends on what you need. In this case, I want all directories within the theme directory without the forward slash at the beginning (e.g. assets). If you do path.sub(site.theme.root, ""), you get /assets and we can't use Pathutil because it doesn't work on Windows the way we're using it.

name = File.basename(path)

if Utils.has_yaml_header?(path)
append_unless_exists site.pages,
Jekyll::Page.new(site, base, dir, name)
else
Copy link
Contributor

Choose a reason for hiding this comment

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

Really elegant implementation.

append_unless_exists site.static_files,
Jekyll::StaticFile.new(site, base, dir, name)
end
end

def append_unless_exists(haystack, new_item)
if haystack.any? { |file| file.relative_path == new_item.relative_path }
Jekyll.logger.debug "Theme:",
"Ignoring #{new_item.relative_path} in theme due to existing file " \
"with that path in site."
return
end

haystack << new_item
end
end
end
12 changes: 8 additions & 4 deletions lib/jekyll/theme.rb
Expand Up @@ -18,15 +18,19 @@ def root
end

def includes_path
path_for :includes
path_for "_includes".freeze
end

def layouts_path
path_for :layouts
path_for "_layouts".freeze
end

def sass_path
path_for :sass
path_for "_sass".freeze
end

def assets_path
path_for "assets".freeze
end

def configure_sass
Expand All @@ -43,7 +47,7 @@ def path_for(folder)
end

def realpath_for(folder)
File.realpath(Jekyll.sanitized_path(root, "_#{folder}"))
File.realpath(Jekyll.sanitized_path(root, folder.to_s))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm torn here. Do we want to pass vanilla symbols (without the _) and special case assets so that we treat all of them as named folders, rather than paths?

Copy link

@ghost ghost Sep 19, 2016

Choose a reason for hiding this comment

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

If we're passing symbols like :_layouts, why is it called realpath_for? It's just like Site#in_theme_dir, except it takes a symbol. I think realpath_for should be an abstraction (special case :assets, for example)

Copy link
Member Author

Choose a reason for hiding this comment

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

Special casing is a non-starter for me in this already complicated codebase. I want a way to fetch the path on disk for a directory. If the directory starts with a _, then let's ask for it that way. It's way more direct to ask for _sass and get _sass then to ask for sass and get _sass.

@spudowiar The difference here is that this uses File.realpath which resolves symlinks. Site#in_theme_dir does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Would a string be a better fit stylistically then?

rescue Errno::ENOENT, Errno::EACCES, Errno::ELOOP
nil
end
Expand Down
2 changes: 1 addition & 1 deletion lib/jekyll/theme_builder.rb
@@ -1,6 +1,6 @@
class Jekyll::ThemeBuilder
SCAFFOLD_DIRECTORIES = %w(
_layouts _includes _sass
assets _layouts _includes _sass
).freeze

attr_reader :name, :path, :code_of_conduct
Expand Down
7 changes: 7 additions & 0 deletions site/_docs/themes.md
Expand Up @@ -27,6 +27,7 @@ Jekyll themes set default layouts, includes, and stylesheets, that can be overri

Jekyll will look first to your site's content, before looking to the theme's defaults, for any requested file in the following folders:

* `/assets`
* `/_layouts`
* `/_includes`
* `/_sass`
Expand Down Expand Up @@ -68,6 +69,12 @@ Theme layouts and includes work just like they work in any Jekyll site. Place la

For example, if your theme has a `/_layouts/page.html` file, and a page has `layout: page` in its YAML front matter, Jekyll will first look to the site's `_layouts` folder for a the `page` layout, and if none exists, will use your theme's `page` layout.

### Assets

Any file in `/assets` will be copied over to the user's site upon build unless they have a file with the same relative path. You may ship any kind of asset here: SCSS, an image, a webfont, etc. These files behave just like pages and static files in Jekyll: if the file has [YAML front matter]({{ site.baseurl }}/docs/frontmatter/) at the top, then it will be rendered. If it does not have YAML front matter, it will simply be copied over into the resulting site. This allows theme creators to ship a default `/assets/styles.scss` file which their layouts can depend on as `/assets/styles.css`.

All files in `/assets` will be output into the compiled site in the `/assets` folder just as you'd expect from using Jekyll on your sites.

### Stylesheets

Your theme's stylesheets should be placed in your theme's `/_sass` folder, again, just as you would when authoring a Jekyll site. Your theme's styles can be included in the user's stylesheet using the `@import` directive.
Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/test-theme/_sass/test-theme-red.scss
@@ -0,0 +1,3 @@
.sample {
color: red;
}
3 changes: 3 additions & 0 deletions test/fixtures/test-theme/assets/application.coffee
@@ -0,0 +1,3 @@
---
---
alert "From your theme."
1 change: 1 addition & 0 deletions test/fixtures/test-theme/assets/img/another-logo.png
Binary file added test/fixtures/test-theme/assets/img/logo.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions test/fixtures/test-theme/assets/style.scss
@@ -0,0 +1,3 @@
---
---
@import "test-theme-{{ site.theme-color | default: "red" }}";
3 changes: 3 additions & 0 deletions test/source/assets/application.coffee
@@ -0,0 +1,3 @@
---
---
alert "From your site."
2 changes: 1 addition & 1 deletion test/test_filters.rb
Expand Up @@ -475,7 +475,7 @@ def to_liquid
g["items"].is_a?(Array),
"The list of grouped items for '' is not an Array."
)
assert_equal 14, g["items"].size
assert_equal 15, g["items"].size
end
end
end
Expand Down
1 change: 1 addition & 0 deletions test/test_site.rb
Expand Up @@ -180,6 +180,7 @@ def generate(site)
%#\ +.md
.htaccess
about.html
application.coffee
bar.html
coffeescript.coffee
contacts.html
Expand Down
10 changes: 5 additions & 5 deletions test/test_theme.rb
Expand Up @@ -34,16 +34,16 @@ def setup
end

context "path generation" do
[:layouts, :includes, :sass].each do |folder|
[:assets, :_layouts, :_includes, :_sass].each do |folder|
should "know the #{folder} path" do
expected = File.expand_path("_#{folder}", @expected_root)
assert_equal expected, @theme.public_send("#{folder}_path")
expected = File.expand_path(folder.to_s, @expected_root)
assert_equal expected, @theme.public_send("#{folder.to_s.tr("_", "")}_path")
end
end

should "generate folder paths" do
expected = File.expand_path("./_sass", @expected_root)
assert_equal expected, @theme.send(:path_for, :sass)
assert_equal expected, @theme.send(:path_for, :_sass)
end

should "not allow paths outside of the theme root" do
Expand All @@ -56,7 +56,7 @@ def setup

should "return the resolved path when a symlink & resolved path exists" do
expected = File.expand_path("./_layouts", @expected_root)
assert_equal expected, @theme.send(:path_for, :symlink)
assert_equal expected, @theme.send(:path_for, :_symlink)
Copy link

Choose a reason for hiding this comment

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

Not happy with this at all, Theme#path_for sounds like it should be an abstraction, looks like it's just a sugary version of Site#in_theme_dir

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not, as I mention above.

end
end

Expand Down
71 changes: 71 additions & 0 deletions test/test_theme_assets_reader.rb
@@ -0,0 +1,71 @@
require "helper"

class TestThemeAssetsReader < JekyllUnitTest
def setup
@site = fixture_site(
"theme" => "test-theme",
"theme-color" => "black"
)
assert @site.theme
end

def assert_file_with_relative_path(haystack, relative_path)
assert haystack.any? { |f|
f.relative_path == relative_path
}, "Site should read in the #{relative_path} file, " \
"but it was not found in #{haystack.inspect}"
end

def refute_file_with_relative_path(haystack, relative_path)
refute haystack.any? { |f|
f.relative_path == relative_path
}, "Site should not have read in the #{relative_path} file, " \
"but it was found in #{haystack.inspect}"
end

context "with a valid theme" do
should "read all assets" do
@site.reset
ThemeAssetsReader.new(@site).read
assert_file_with_relative_path @site.static_files, "assets/img/logo.png"
assert_file_with_relative_path @site.pages, "assets/style.scss"
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to test that it doesn't clobber site files?

Copy link
Member Author

Choose a reason for hiding this comment

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

Clobber?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I have /assets/foo.js in my site, and /assets/foo.js in my theme, to confirm /assests/foo.js in the resulting site contain's the site's content, not the theme's.


should "convert pages" do
@site.process

file = @site.pages.find { |f| f.relative_path == "assets/style.scss" }
refute_nil file
assert_equal @site.in_dest_dir("assets/style.css"), file.destination(@site.dest)
assert_includes file.output, ".sample {\n color: black; }"
end

should "not overwrite site content with the same relative path" do
@site.reset
@site.read

file = @site.pages.find { |f| f.relative_path == "assets/application.coffee" }
refute_nil file
assert_includes file.content, "alert \"From your site.\""
end
end

context "with a valid theme without an assets dir" do
should "not read any assets" do
site = fixture_site("theme" => "test-theme")
allow(site.theme).to receive(:assets_path).and_return(nil)
ThemeAssetsReader.new(site).read
refute_file_with_relative_path site.static_files, "assets/img/logo.png"
refute_file_with_relative_path site.pages, "assets/style.scss"
end
end

context "with no theme" do
should "not read any assets" do
site = fixture_site("theme" => nil)
ThemeAssetsReader.new(site).read
refute_file_with_relative_path site.static_files, "assets/img/logo.png"
refute_file_with_relative_path site.pages, "assets/style.scss"
end
end
end