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

rake assets:precompile grabs all .less files #35

Closed
pacovell opened this issue Apr 9, 2012 · 27 comments
Closed

rake assets:precompile grabs all .less files #35

pacovell opened this issue Apr 9, 2012 · 27 comments

Comments

@pacovell
Copy link

pacovell commented Apr 9, 2012

application.css.less:

//= require widget

widget.css.less:

@import 'widget/variables'
@import 'widget/header'

app/assets/stylesheets/widget has variables.less and header.less.

The engine is processing the widget directory in alphabetical order, paying no attention to the application or widget manifests. If I remove the require or import statements entirely, the widget directory is still processed.

The problem arises if header.less uses variables.less, then it is undefined because the processing is in alphabetical order instead of manifest order; then I get an undefined variable error:

variable @xyz is undefined

I think this is in error, since only files included in the manifest somewhere should be included. I investigated by adding a puts to the default precompile proc:

 config.assets.precompile = [ Proc.new { |path| 
   puts path
   !File.extname(path).in?(['.js', '.css']) 
}, /(?:\/|\\|\A)application\.(css|js)$/ ]

The filename (path) passed to the proc is 'variables' not 'variables.less'. If I compare to a sass-rails configuration, I see that the equivalent 'variables.sass' file is passed to the precompile proc as 'variables.css' so it is NOT compiled, yielding the proper behavior.

I'm not exactly sure where to fix this, but I'm happy to submit a pull if you can point me in the right direction. My workaround for the moment is to add '' (filenames without extensions) to the precompile proc exclude list.

@metaskills
Copy link
Owner

I just looked at sass-rails and could not see anything passed to the upper host rails application for assets precompile. Just basic configs in their dummy application. Can you point me to specific places and examples where you see sass-rails doing things that fixes this problem in their end? If so, we could mimic and it would be a good fix for us.

@pacovell
Copy link
Author

pacovell commented Apr 9, 2012

Yeah, I looked in the same places. My guess right now is that they're doing it in the resolver - does that make sense to you?

@pacovell
Copy link
Author

pacovell commented Apr 9, 2012

importer.rb:33

def resolve(name, base_pathname = nil)
      name = Pathname.new(name)
      if base_pathname && base_pathname.to_s.size > 0
        root = Pathname.new(context.root_path)
        name = base_pathname.relative_path_from(root).join(name)
      end
      partial_name = name.dirname.join("_#{name.basename}")
      @resolver.resolve(name) || @resolver.resolve(partial_name)
end

They also distinguish between a partial (starts with _) and non-partial. I'm not sure this is relevant or helpful in our case.

@djholt
Copy link

djholt commented Apr 11, 2012

For what it's worth, I'm seeing the same issue. I see undefined variable errors because individual .less files are being precompiled. Everything works fine in development. I plan to try pacovell's workaround.

@bbhoss
Copy link

bbhoss commented Apr 18, 2012

What needs to be done here? We ran into this issue today, and would love to fix it, or contribute towards a fix.

@metaskills
Copy link
Owner

For someone to fully investigate this issue and report how either sprockets and/or sass-rails handles this issue from their side. From what I can see, individual .scss files are not precompiled.

I just did a quick test using the less-rails-bootstrap-test project and found that none of these individual files located in the basic_less or full_control are processed.

https://github.com/metaskills/less-rails-bootstrap-test/tree/master/app/assets/stylesheets

So perhaps people are just doing it wrong against less-rails as advertised usage? Either way, some one has to demonstrate the problem.

@bbhoss
Copy link

bbhoss commented Apr 18, 2012

Ok, so step 1, let's get a demo app going of the issue. We'll look at how sass-rails handles this too. I have experienced this issue locally, it seems to be worked around by removing the default proc that comes with rails (as it includes all assets except .css and .js (and application(.js|.css)) by default). The weird thing is, in the proc, if you print out the path passed into the proc, it shows less files as .css, but it also does this for sass, so I'm not sure if it matters.

@metaskills
Copy link
Owner

Just so you know, just making an app that shows the issue doe not mean it is a real issue. Like I was saying, you could be doing it wrong in how you layout the file. So far less-rails-bootstrap-test does not exhibit what your talking about and that of me is the gospel of how you should structure your pipeline files using less.

@bbhoss
Copy link

bbhoss commented Apr 18, 2012

Please see my fork of your test, I have the issue demonstrated there (added in this commit: bbhoss/less-rails-bootstrap-test@b77ed5e). Also, note that this all works in development mode, you only discover the failure when you precompile. For this to be designed behavior it seems quite odd, as Twitter bootstrap itself uses this paradigm (it uses mixins in files that are included below the mixins include in the main bootstrap.css). Thanks.

@metaskills
Copy link
Owner

Would it work if the file name is custom.css.less?

@pacovell
Copy link
Author

As I wrote above, I believe this is the key:

The filename (path) passed to the proc is 'variables' not 'variables.less'. If I compare to a sass-rails configuration, I see that the equivalent 'variables.sass' file is passed to the precompile proc as 'variables.css' so it is NOT compiled, yielding the proper behavior.

So somewhere, sass-rails is passing variables.css when it translates the name from variables.sass, but less-rails is passing variables without any extension. If we can find where that extension is removed and change it to .css instead, we will have the same behavior as sass-rails.

The REASON you don't see the problem in less-rails-bootstrap is NOT because it isn't happening. It's because the two .less files, mixins and variables, don't have any dependencies on other files that must be loaded first, so it doesn't matter that they are loaded at any time.

@metaskills
Copy link
Owner

Are you sure? It looks to me like mixins.less needs to have the .border-radius defined elsewhere.

https://github.com/metaskills/less-rails-bootstrap-test/blob/master/app/assets/stylesheets/full_control/mixins.less

@pacovell
Copy link
Author

I added the above code to print out the assets. This is with a fresh clone of less-rails-bootstrap-test.

$ rake assets:precompile
/usr/local/bin/ruby /usr/local/bin/rake assets:precompile:all RAILS_ENV=production RAILS_GROUPS=assets
rails.png
application.js
basic.css
basic_less.css
basic_less/index.css
full_control.css
full_control/index.css
full_control/mixins
full_control/variables
twitter/bootstrap/glyphicons-halflings-white.png
twitter/bootstrap/glyphicons-halflings.png
twitter/bootstrap.js
// ... snip ...

@pacovell
Copy link
Author

@metaskills I suspect because .border-radius() itself is included in a mixin, it may not be evaluated at that point. You can see from the printout above that the files are definitely being processed.

@metaskills
Copy link
Owner

OK, I see an empty full_control/mixins. Did not cause an error here tho. Let me know what more y'all find. Will patch if this get's nailed down.

@pacovell
Copy link
Author

For the elimination of any doubt, if I add

background-color: @grayLight;

to variables.less, it will crash as below. @GrayLight is defined by twitter bootstrap variables.

$ rake assets:precompile
/usr/local/bin/ruby /usr/local/bin/rake assets:precompile:all RAILS_ENV=production RAILS_GROUPS=assets
rails.png
application.js
basic.css
basic_less.css
basic_less/index.css
full_control.css
full_control/index.css
full_control/mixins
full_control/variables
rake aborted!
variable @grayLight is undefined
  (in /Users/pac/server/git/less-rails-bootstrap-test/app/assets/stylesheets/full_control/variables.less)

Tasks: TOP => assets:precompile:primary
(See full trace by running task with --trace)

@bbhoss
Copy link

bbhoss commented Apr 18, 2012

@metaskills I tried custom.less, no dice, same error. @pacovell we worked around this locally by not precompiling any .less files, except the ones we specified. When we specify precompile to be just full_control.css.less (yes, we added the .less extension), it compiles as expected.

Also, @metaskills I found an issue in the generated CSS:

.icon-white{background-image:url("url(/assets/twitter/bootstrap/glyphicons-halflings-white-4fbb6a0b9b4e61912f486ac494a858f1.png)");}

@pacovell
Copy link
Author

@bbhoss My workaround looks like this:

config.assets.precompile = [ Proc.new { |path| 
   !File.extname(path).in?(['.js', '.css', '']) 
}, /(?:\/|\\|\A)application\.(css|js)$/ ]

.css.less files work fine (without any mod) because when the .less extension is stripped, it becomes .css, which is ignored by the default precompile function.

@hayesr
Copy link

hayesr commented Apr 23, 2012

Interested in this issue as well.

@benjaminws
Copy link

Also ran into this issue. The workaround from @pacovell works for me.

@bronson
Copy link

bronson commented Apr 26, 2012

Just ran into this too. Huge thanks to @pacovell, the workaround works great.

@ktkaushik
Copy link

this worked for me.

config.assets.compile = [ Proc.new { |path| 
   !File.extname(path).in?(['.js', '.css', '']) 
  }, /(?:\/|\\|\A)application\.(css|js)$/ ]

@iainbeeston
Copy link

I get this issue locally as well. I'm using a variant of the workaround given by @pacovell

@drn
Copy link

drn commented May 24, 2012

Awesome, thanks @pacovell ! I've been stuck on the same issue for a while.

@swistak
Copy link

swistak commented May 27, 2012

Same issue here. This is a real problem. Fact that @metaskills can find a case where it does not happen does not mean it does not exist :/

It grabs every file that it can find, sass works around this becouse partials there start with _ so they skip them, in .less it's not that easy.

@pacovell Thanks for a workaround

@metaskills
Copy link
Owner

OK, these work arounds were just that. I was hoping someone would do the legwork and keep digging on top of @pacovell's great work and find the source of the issue. Here is the solution I came up with today, after an hour of digging.

I found that sprockets each_logical_path enumerator would have files like bootstrap/_close.scss yielded as bootstrap/_close.css. So I dug down to the Hike gem which is the trail object and found that in a default rails application with Sass that the trails aliases would have an entry like so Rails.application.assets.send(:trail).aliases['.less'] # => '.css'. However in the test less/bootstrap projects, this entry was nil. So I set out to find why the sprockets/tilt defaults were being lost.

Along those lines, I found that the sprockets/procesing.rb file had this method below.

def add_engine_to_trail(ext, klass)
  @trail.append_extension(ext.to_s)
  if klass.respond_to?(:default_mime_type) && klass.default_mime_type
    if format_ext = extension_for_mime_type(klass.default_mime_type)
      @trail.alias_extension(ext.to_s, format_ext)
    end
  end
end

And I know the less-rails' Less::Rails::LessTemplate subclasses Tilt::LessTemplate and that super classes template handler does have a self.default_mime_type = 'text/css', but I guess class variables do not super up or respond to properly. So like sass-rails, I added that line to less-rails template handler and now I can see when I inspect the assets trail aliases that it has a valid ".less" key and value of ".css". So that means that this commit d18153e should fix the problem. So bundle update less-rails and let me know.

@metaskills
Copy link
Owner

Closing this issue, remember, just run $ bundle update less-rails and make sure you are on version 2.2.3 or higher.

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

No branches or pull requests