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

Conversation

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented Dec 1, 2018

Fix backwards-incompatible change in 4.10’s Sass filter.

Detailed description

When no load paths are given to the Sass filter, it should still be able to resolve relative paths (e.g. ../external) and successfully import. This isn’t currently possible, because the Sass filter assumes that the files have a corresponding Nanoc item.

This change allows the Sass filter to (again) import such files.

To do

(Include the to-do list for this PR to be finished here.)

  • Tests

Related issues

Fixes #1378.

@ddfreyne ddfreyne force-pushed the sass-load-even-if-file-does-not-exist branch from 459d701 to 683b306 Dec 1, 2018
@@ -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.

Loading

Copy link
Member Author

@ddfreyne ddfreyne Dec 1, 2018

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'

Loading

options[:importer] = self
::Sass::Engine.new(item.raw_content, options)
::Sass::Engine.new(content, options)
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.

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

Loading

@@ -0,0 +1,25 @@
# frozen_string_literal: true

describe 'GH-1378', site: true, stdio: true do
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.

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

Loading

Copy link
Member Author

@ddfreyne ddfreyne Dec 1, 2018

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.

Loading

@ddfreyne ddfreyne merged commit 58dd66a into master Dec 1, 2018
3 checks passed
Loading
@ddfreyne ddfreyne deleted the sass-load-even-if-file-does-not-exist branch Dec 1, 2018
@ddfreyne
Copy link
Member Author

@ddfreyne ddfreyne commented Dec 1, 2018

As suggested by @gpakosz: a useful refactoring later on might be to have two importer classes — one that deals with Nanoc items exclusively, and one that deals with importing relative files not belonging to a Nanoc item. That’d break up the responsibilities of the two importers a bit more nicely; currently, the single importer does a bit too much.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants