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

Allow Sass to import files even if they don’t have an item #1379

Merged
merged 1 commit into from
Dec 1, 2018
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 6 additions & 5 deletions nanoc/lib/nanoc/filters/sass/importer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,16 @@ def find_relative(name, base_identifier, options)
return unless raw_filename

item = raw_filename_to_item(raw_filename)
# it doesn't make sense to import a file, from Nanoc's content if the corresponding item has been deleted
raise "unable to map #{raw_filename} to any item" if item.nil?
Copy link
Member

@gpakosz gpakosz Dec 1, 2018

Choose a reason for hiding this comment

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

Alright, this line indeed makes the compilation stop.

I think you should replace

# it doesn't make sense to import a file, from Nanoc's content if the corresponding item has been deleted
raise "unable to map #{raw_filename} to any item" if item.nil?

by

# the imported file doesn't correspond to a Nanoc item, let next importer in Sass' :load_paths handle it
return if item.nil?

And keep the rest of importer.rb as it is.

This suggestion alone fixes #1378.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed on Gitter, this won’t work with paths that are relative to the item itself and live outside of the Nanoc content directory.

e.g. current item = content/style/foo.sass, path to import = `../../external.sass'


filter.depend_on([item])
content = item ? item.raw_content : File.read(raw_filename)
filename = item ? item.identifier.to_s : raw_filename

filter.depend_on([item]) if item

options[:syntax] = syntax
options[:filename] = item.identifier.to_s
options[:filename] = filename
options[:importer] = self
::Sass::Engine.new(item.raw_content, options)
::Sass::Engine.new(content, options)
Copy link
Member

Choose a reason for hiding this comment

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

I understand why you're doing it. Indeed it may be desirable to keep backward compatibility and let people @import with relative paths where imported files live outside of Nanoc's content/ directory instead of importing relatively from a configured entry in load_paths

end

def find(identifier, options)
Expand Down
13 changes: 11 additions & 2 deletions nanoc/spec/nanoc/filters/sass_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,13 @@
rep.raw_paths = rep.paths = { last: [Dir.getwd + '/output/style/main.sass'] }
end
end

let(:item_main_sourcemap_rep) do
Nanoc::Int::ItemRep.new(item_main, :sourcemap).tap do |rep|
rep.raw_paths = rep.paths = { last: [Dir.getwd + '/output/style/main.sass.map'] }
end
end

let(:item_main_view) { Nanoc::CompilationItemView.new(item_main, view_context) }
let(:item_main_default_rep_view) { Nanoc::CompilationItemRepView.new(item_main_default_rep, view_context) }
let(:item_main_sourcemap_rep_view) { Nanoc::CompilationItemRepView.new(item_main_sourcemap_rep, view_context) }
Expand Down Expand Up @@ -122,9 +124,11 @@
reps << item_main_sourcemap_rep
end
end

let(:dependency_tracker) { Nanoc::Int::DependencyTracker.new(dependency_store) }
let(:dependency_store) { Nanoc::Int::DependencyStore.new(empty_items, empty_layouts, config) }
let(:compilation_context) { double(:compilation_context) }

let(:snapshot_repo) do
Nanoc::Int::SnapshotRepo.new.tap do |repo|
repo.set(reps[item_blue].first, :last, Nanoc::Int::TextualContent.new('.blue { color: blue }'))
Expand Down Expand Up @@ -229,7 +233,7 @@
before { File.write('_external.scss', 'body { font: 100%; }') }

context 'load_path set' do
it 'can import by relative path' do
it 'can import (using load paths) by relative path' do
expect(sass.setup_and_run('@import external', load_paths: ['.']))
.to match(/\Abody\s+\{\s*font:\s+100%;?\s*\}\s*\z/)
end
Expand All @@ -241,10 +245,15 @@
end

context 'load_path not set' do
it 'cannot import by relative path' do
it 'cannot import (using load paths) by relative path' do
expect { sass.setup_and_run('@import external') }
.to raise_error(::Sass::SyntaxError, /File to import not found/)
end

it 'can import (using importer) by relative path' do
expect(sass.setup_and_run('@import "../../_external"'))
.to match(/\Abody\s+\{\s*font:\s+100%;?\s*\}\s*\z/)
end
end
end

Expand Down
25 changes: 25 additions & 0 deletions nanoc/spec/nanoc/regressions/gh_1378_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

describe 'GH-1378', site: true, stdio: true do
Copy link
Member

Choose a reason for hiding this comment

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

Totally cool to learn how to assemble a "real site" instead of having to mock all Nanoc internals 👍

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 in particular used for regression tests, although I have also started writing integration tests in spec/nanoc/integration.

It’s not a substitute for unit tests, though, because the integration/regression tests tend to run slowly and so it’s not great for testing all the different cases.

before do
FileUtils.mkdir_p('content')
File.write('outside.scss', 'p { color: red; }')
File.write('content/style.scss', '@import "../outside.scss";')

File.write('Rules', <<~EOS)
compile '/*' do
filter :sass, syntax: :scss
write ext: 'css'
end
EOS
end

example do
expect { Nanoc::CLI.run([]) }
.to change { File.file?('output/style.css') }
.from(false)
.to(true)

expect(File.read('output/style.css')).to match(/p\s*{\s*color:\s*red;\s*}/)
end
end