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

patches/sass_importer incorrect method usage #195

Closed
danielpclark opened this issue Dec 14, 2014 · 28 comments · Fixed by #201
Closed

patches/sass_importer incorrect method usage #195

danielpclark opened this issue Dec 14, 2014 · 28 comments · Fixed by #201

Comments

@danielpclark
Copy link

https://github.com/Compass/compass-rails/blob/master/lib/compass-rails/patches/sass_importer.rb

  • #(1) wrong number of arguments (2 for 1) for line 12 in sass_importer.rb for SassImporter.new
paths += context.environment.paths.map { |path| self.class.parent::SassImporter.new(context, path) }

and line 21

:importer => self.class.parent::SassImporter.new(context, context.pathname),

I tracked this down. Each library brings the class in via SassImporter < Sass::Importers::Filesystem. In Sass the code reads as follows:

module Sass
  module Importers
    # The default importer, used for any strings found in the load path.
    # Simply loads Sass files from the filesystem using the default logic.
    class Filesystem < Base
      attr_accessor :root
      # Creates a new filesystem importer that imports files relative to a given path.
      #
      # @param root [String] The root path.
      # This importer will import files relative to this path.
      def initialize(root)
        @root = File.expand_path(root)
        @real_root = Sass::Util.realpath(@root).to_s
        @same_name_warnings = Set.new
      end
      # ...
    end
  end
end

Source: https://github.com/sass/sass/blob/stable/lib/sass/importers/filesystem.rb#L14


  • #(2) wrong number of arguments (1 for 2) for line 10 in sass_importer.rb for SassCacheStore.new
cache_store = Sprockets::SassCacheStore.new(context.environment)

For (2) the source method reads:

class CacheStore < ::Sass::CacheStores::Base
  VERSION = '1'
  def initialize(cache, version)
    @cache, @version = cache, "#{VERSION}/#{version}"
  end
  # ...
end

Source: https://github.com/sstephenson/sprockets/blob/2c9333bd1b6a55f791a3f2ca1389bf152cea18ba/lib/sprockets/sass_processor.rb#L260

I'm using Ruby 2.2.0.preview1, Rails 4.2.0.rc3. And using:
gem 'compass-rails', '> 2.0.1'
gem 'sprockets', '
> 3.0.0.beta.6'
gem 'sass-rails', '> 5.0.0'
gem 'sass', '
> 3.4.9'

danielpclark added a commit to danielpclark/compass-rails that referenced this issue Dec 15, 2014
danielpclark added a commit to danielpclark/compass-rails that referenced this issue Dec 15, 2014
danielpclark added a commit to danielpclark/compass-rails that referenced this issue Dec 15, 2014
@ollym
Copy link

ollym commented Dec 17, 2014

This is giving me: uninitialized constant Sprockets::SassProcessor

For some reason, Sprockets::SassProcessor isn't autoloading in 3.0.0.beta.6

@danielpclark
Copy link
Author

@ollym Do you have the line number and the source file in your error message?

tvdeyen pushed a commit to AlchemyCMS/alchemy_cms that referenced this issue Dec 18, 2014
This fixes the "wrong number of arguments" error from latest compass-rails 2.0.1 used together with sprockets 2.12.3.

See: Compass/compass-rails#195
@CamJN
Copy link

CamJN commented Dec 18, 2014

This is causing spree-frontend to error out too. Same wrong number of arguments (2 for 1) error from compass-rails, same line in compass-rails (2.0.1) lib/compass-rails/patches/sass_importer.rb:12.

@salimane
Copy link
Contributor

+1

@danielpclark
Copy link
Author

For anyone having this issue I have a pull request that fixes this #197

Feel free to set your Rails gemfile with the line gem 'compass-rails', github: 'danielpclark/compass-rails' and use the working code.

@rubyistdotjs
Copy link

@danielpclark solution worked for me.
But now I run into an other issue: when I change something in a partial, application.scss isn't recompiled.
The issue seems to come from this gem (maybe from your changes?), since when I take it off everything works as expected.

# Gemfile
ruby '2.2.0'

gem 'rails', '4.2.0.rc3'
gem 'sass-rails', '~> 5.0.0'
gem 'compass-rails', github: 'danielpclark/compass-rails'
gem 'susy', '~> 2.1.0'
// application.scss
@charset "utf-8";

@import "compass/css3";
@import "compass/utilities";
@import "susy";

@import "scaffolding"
// _scaffolding.scss
body {
  color: #333;
  font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
  font-size: 14px;
  line-height: 20px;
  background-color: #fff;
}

Change background-color and nothing happens. Now comment the gem and the two import compass lines, bundle install and relaunch the server and now the changes should trigger a recompile.

I should also mention that compass-core throw me a warning: caniuse.rb:72: warning: circular argument reference - browsers. But it doesn't look relevant.

@danielpclark
Copy link
Author

@colderis I believe I know why this may be happening. In my code I used Sprockets::Cache::VERSION as the version parameter. I'll look into putting the actual compiled sass resource(s) version info here. This will, should, fix your issue with not updating compiled resources. Let me look into it and I'll get back to you.

@danielpclark
Copy link
Author

@colderis Alright I've fixed it. I've used the same context.environment variable that was used in the previous method/version. Now any changes in your CSS/SCSS files get updated immediately.

@rubyistdotjs
Copy link

It didn't came from your changes. Actually the line you mention wasn't called at all.

This code does the same thing for me:

def evaluate(context, locals, &block)
  cache_store = self.class.parent::CacheStore.new(context.environment)
  paths  = context.environment.paths.map { |path| CompassRails::SpriteImporter.new(context, path) }
  paths += context.environment.paths.map { |path| self.class.parent::SassImporter.new(path) }
  paths += ::Rails.application.config.sass.load_paths

  options = CompassRails.sass_config.merge( {
    :filename => eval_file,
    :line => line,
    :syntax => syntax,
    :cache_store => cache_store,
    :importer => self.class.parent::SassImporter.new(context.pathname),
    :load_paths => paths,
    :sprockets => {
      :context => context,
      :environment => context.environment
    }
  })

  ::Sass::Engine.new(data, options).render
rescue ::Sass::SyntaxError => e
  # Annotates exception message with parse line number
  context.__LINE__ = e.sass_backtrace.first[:line]
  raise e
end

What fixes the issue is replacing ::Sass::Engine.new(data, options).render by:

engine = ::Sass::Engine.new(data, options)
css = engine.render

engine.dependencies.map do |dependency|
  context.depend_on(dependency.options[:filename])
end

css

Then I just filtered, within the map block, everything that doesn't come from the rails app root folder. Otherwise all the compass and susy files imported in application.scss are added to depend_on which leads to a huge performance problem.

I'll try to find a proper solution.

@danielpclark
Copy link
Author

The code you've pasted is from the main source without any of my changes. My tests in my development environment work for me.

I don't know of an easy way of detecting what's going on in it. I've just been reading the code and going with what I understand of it. Then I've tried to see what works. Could you share with me how you know about:

the line you mention wasn't called at all

and

Otherwise all the compass and susy files imported in application.scss are added to depend_on which leads to a huge performance problem.

The original errors I pasted here showed that these lines (the ones I've updated) are called. I was unable to use the pry gem to break into the code execution and test the local environment variables and state. So I really would like to know how you're finding out such things. It'll be a great help!

yesmeck added a commit to yesmeck/compass-rails that referenced this issue Dec 21, 2014
Fixes Compass#195 (works with latest Sass and Sprockets)
@danielpclark
Copy link
Author

My pull request currently loads application.css just fine. The require's aren't getting pulled in though. It needs to be updated further.

@danielpclark
Copy link
Author

@colderis I believe you are right about where the problem now lies. I've been looking into ::Sass::Engine.new(data, options).render and it seems the data object is created in the Railtie. The engine requires data to be a template. From what I can see render is working with a singular template resource. Options handles paths with options[:load_paths]. The load paths go through a normalization. But I haven't found anyplace where files are loaded from source. I'll continue looking into it.

...
After looking further; your dependencies method example looks like the right call. I would have figured it would be incorporated in the code already... I'm not seeing it.

@DavidBennettPIO
Copy link

Hi Guys, I've been having the same 2 bugs, the first @danielpclark highlighted in this issue for about a week and the second one @colderis reported for about 2 months now.

My 2 cents:

The second poped up 2 months ago for me when I needed to use sass 3.3.x ... and in-turn needed to force sass-rails to 5.0.0.beta and compass-core to >= 1.0.1.

@danielpclark's pull fixes the wrong number of arguments (2 for 1) error for me. But I still need to remove my tmp/cache/assets directory and restart the server to see any changes :(

Thanks for all your hard work!

@danielpclark
Copy link
Author

Thanks @complistic-gaff. When you say you need to delete temp files and restart the server is that in the development environment, or just production?

@colderis I've found a tool to debug with: byebug is an execllent gem recommended in the Rails docs.

Looking at the data parameter that gets handed to the ::Sass::Engine I see that it's just the main application.css file. So even though file paths are handed in with options param, it seems the CSS/SCSS data has (already) been grabbed. So I'm thinking we need to either have the data object updated with all the relative content beforehand, or modify the content as you have.

Could you tell me what you expect the behavior to be when it works rather than:

Otherwise all the compass and susy files imported in application.scss are added to depend_on which leads to a huge performance problem.

What role is depend_on supposed to perform here?

@DavidBennettPIO
Copy link

@danielpclark delete/restart in development... in production I need to precompile/restart (as normal).

FYI: I have a main application.css.sass that looks like this:

@import "_fonts"
@import "_framework_and_overrides"
@import "_layout"
@import "_facilities"

Then each of the sass files includes the compass bits it needs.

@danielpclark
Copy link
Author

@complistic-gaff Are your @imports loading after a compile/restart? I'm currently not getting any *= require resources to load. Just application.css.

@DavidBennettPIO
Copy link

@danielpclark, Everything loads and works fine after I delete the temp files then restart the server.

If I don't do that, only updates to the main application.css.sass file are applied and any updates to the @import's are not compiled.

For example if I make changes to _layout.sass these changed are not visible in the compiled application.css.

@DavidBennettPIO
Copy link

@danielpclark If I make a change to both application.css.sass and _layout.sass both changes are compiled as I would expect.

This sounds like _layout.sass is not being registered as a dependency to application.css.sass and so does not invalidate the cache.

I will try Monkey Patching the code in from @colderis to see is it helps.

@DavidBennettPIO
Copy link

Yep, forking your code and adding @colderis patch "fixed" the problem for me.

@rubyistdotjs
Copy link

@danielpclark yes I used byebug. And the first block of code is simply the fix for the first issue without all the backward compatibility stuff (I just realised it's pretty much what @hsbt wrote in his pull request #194).

The problem is simple, compass-rails isn't compatible with sass-rails 5.x.
I would recommend switching to bourbon, if you can, until compass-rails catch up.

@danielpclark
Copy link
Author

@complistic-gaff I looked more into the patch @colderis shared with me which comes from rails/sass-rails#283 . I'm thinking there might be a little more to do then just

engine = ::Sass::Engine.new(data, options)
css = engine.render

engine.dependencies.map do |dependency|
  context.depend_on(dependency.options[:filename])
end

css

The context of this original patch had other work involved.

I'll only be working on this during weekends. So more later.

@DavidBennettPIO
Copy link

@danielpclark yeah I just wanted to confirm that I have both of your problems, and that @colderis issue popped up when I started to use sass 3.3.x and sass-rails 5.0.0.beta almost 2 months ago, long before the one highlighted in the OP.

I agree that there is more to be done on the dependencies issue, just wanted to give some background and comformation.

@cflipse
Copy link

cflipse commented Dec 24, 2014

The breaking change is in sass-rails -- Between v5.0.0.beta1 and v5.0.0, it stopped stomping on the inherited initializer. Downgrading sass-rails to v5.0.0.beta1 fixes the issue for me, temporarily.

rails/sass-rails@91f7509eb12d3e3f9f9dc564570f8f5dbf3b8c0aL141

rmm5t pushed a commit to rmm5t/compass-rails that referenced this issue Dec 30, 2014
@rmm5t
Copy link
Contributor

rmm5t commented Dec 30, 2014

Please see #201. I cherry-picked and squashed @danielpclark's work and also turned the test suite green while added new tests to cover the scenario that everyone is currently suffering from.

I could use some help reviewing and testing the pull-request:

gem "compass-rails", github: "rmm5t/compass-rails", branch: "sass-rails-5.0" 

UPDATE: The PR was merged. Use this instead:

gem "compass-rails", github: "Compass/compass-rails", branch: "master" 

@cflipse
Copy link

cflipse commented Dec 30, 2014

Fix confirmed. (sorry, previous phrasing was unclear.)

@danielpclark
Copy link
Author

I can confirm it @cflipse. If you put this in your gemfile

gem "compass-rails", github: "Compass/compass-rails", branch: "master" 

then this issue has been repaired. The current gem 2.0.1 does not include this patch.

The discussion further down in this thread about secondary CSS/SCSS files not being included has been moved into new issue #208

@bwilkn
Copy link

bwilkn commented Jan 5, 2015

+1 on fix, cheers.

@craigmcnamara
Copy link
Member

Use version 2.0.2

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

Successfully merging a pull request may close this issue.

10 participants