Skip to content

Commit

Permalink
Set the standard :css_filename option for sass. This enables relative…
Browse files Browse the repository at this point in the history
… path calculations for assets referred to by the stylesheet.
  • Loading branch information
chriseppstein committed Dec 5, 2011
1 parent 005e6eb commit 0b43583
Showing 1 changed file with 5 additions and 2 deletions.
7 changes: 5 additions & 2 deletions lib/sass/rails/template_handlers.rb
Expand Up @@ -19,7 +19,7 @@ def resolve(path, content_type = :self)
nil
end

def public_path(path, scope)
def public_path(path, scope = nil)
context.asset_paths.compute_public_path(path, ::Rails.application.config.assets.prefix)
end

Expand Down Expand Up @@ -72,14 +72,17 @@ def sass_options(scope)
options = sass_options_from_rails(scope)
load_paths = (options[:load_paths] || []).dup
load_paths.unshift(importer)
resolver = Resolver.new(scope)
css_filename = File.join(::Rails.public_path, resolver.public_path(scope.logical_path)) + ".css"
options.merge(
:filename => eval_file,
:css_filename => css_filename,
:line => line,
:syntax => syntax,
:importer => importer,
:load_paths => load_paths,
:custom => {
:resolver => Resolver.new(scope)
:resolver => resolver
}
)
end
Expand Down

9 comments on commit 0b43583

@jeremy
Copy link
Member

@jeremy jeremy commented on 0b43583 Dec 22, 2011

Choose a reason for hiding this comment

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

This led to a circular require regression in my app. No luck coming up with a minimal reproduction, yet, though.

@chriseppstein
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a pretty simple refactoring. It's hard to understand how that could happen.

@jeremy
Copy link
Member

@jeremy jeremy commented on 0b43583 Dec 23, 2011

Choose a reason for hiding this comment

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

Happens when precompiling assets. Frustratingly, it's easy to repro in my app, but I couldn't get a minimal repro going in sass-rails tests or in a sample app.

I'm referencing an asset path in foo.js.erb: <%= asset_path 'application.css' %>. This JavaScript is compiled before the stylesheet, so, after this commit, referencing the asset path forces application.css to be compiled.

Any import in application.css throws a circular require error. With just @import "foo"; in application.css, I see asset lookups for both foo and foo.css logical paths. These resolve to the same path, hence the error.

Pretty confusing; I'll need to step through in a debugger to see what's actually going on.

@jeremy
Copy link
Member

@jeremy jeremy commented on 0b43583 Dec 23, 2011

Choose a reason for hiding this comment

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

Looks like someone else reproduced @ rails/rails#4151

@chriseppstein
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a bug with sprockets then. Why does getting the public path cause compilation? I need to tell sass what the css filename is going to be so that relative path computation can be performed. Is there a different sprockets method I should call?

@jeremy
Copy link
Member

@jeremy jeremy commented on 0b43583 Dec 23, 2011 via email

Choose a reason for hiding this comment

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

@chriseppstein
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be ok if it was just the non-digested version of the filename for now. Is there a way of getting at that without compiling and without sass-rails making assuptions about sprockets internals?

@jeremy
Copy link
Member

@jeremy jeremy commented on 0b43583 Dec 24, 2011

Choose a reason for hiding this comment

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

Yep: 7cd6c0e

@chriseppstein
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent.

Please sign in to comment.