Merge pull request #2244 from vikhyat/css-import

Site Customization @import fixes

app/assets/stylesheets/common.scss

@@ -8,3 +8,6 @@
 @import "common/components/*";
 @import "common/admin/*";
 @import "common/input_tip";
+
+/* This file doesn't actually exist, it is injected by DiscourseSassImporter. */
+@import "plugins";

app/assets/stylesheets/mobile.scss

@@ -1,2 +1,5 @@
 @import "common";
 @import "mobile/*";
+
+/* This file doesn't actually exist, it is injected by DiscourseSassImporter. */
+@import "plugins_mobile";

app/assets/stylesheets/plugins.css.erb

@@ -1,7 +0,0 @@
-<%
-  # TODO this is very tricky, we want to add a dependency here on files that may not yet exist
-  #  otherwise in dev we are often stuck nuking the tmp/cache directory
-  DiscoursePluginRegistry.stylesheets.each do |css|
-    require_asset(css)
-  end
-%>

app/assets/stylesheets/plugins_mobile.css.erb

@@ -1,7 +0,0 @@
-<%
-  # TODO this is very tricky, we want to add a dependency here on files that may not yet exist
-  #  otherwise in dev we are often stuck nuking the tmp/cache directory
-  DiscoursePluginRegistry.mobile_stylesheets.each do |css|
-    require_asset(css)
-  end
-%>

app/models/site_customization.rb

@@ -14,12 +14,26 @@ class SiteCustomization < ActiveRecord::Base
   end
 
   def compile_stylesheet(scss)
+    # Get the sprockets environment. In production Rails.application.assets is a
+    # Sprockets::Index instead of Sprockets::Environment, there is no cleaner way
+    # to get the environment from the index.
+    env = Rails.application.assets
+    if env.is_a?(Sprockets::Index)
+      env = env.instance_variable_get('@environment')
+    end
+
+    context = env.context_class.new(env, "custom.scss", "app/assets/stylesheets/custom.scss")
+
     ::Sass::Engine.new(scss, {
       syntax: :scss,
       cache: false,
       read_cache: false,
       style: :compressed,
-      filesystem_importer: DiscourseSassImporter
+      filesystem_importer: DiscourseSassImporter,
+      sprockets: {
+        context: context,
+        environment: context.environment
+      }
     }).render
 
   rescue => e

app/views/common/_discourse_stylesheet.html.erb

@@ -1,10 +1,8 @@
 <%- unless SiteCustomization.override_default_style(session[:preview_style]) %>
   <% if mobile_view? %>
     <%= stylesheet_link_tag "mobile" %>
-    <%= stylesheet_link_tag "plugins_mobile" %>
   <% else %>
     <%= stylesheet_link_tag "desktop" %>
-    <%= stylesheet_link_tag "plugins" %>
   <% end %>
 <%- end %>
 

config/application.rb

@@ -68,7 +68,7 @@ module Discourse
       path =~ /assets\/images/ && !%w(.js .css).include?(File.extname(filename))
     end]
 
-    config.assets.precompile += ['vendor.js', 'common.css', 'desktop.css', 'mobile.css', 'admin.js', 'admin.css', 'shiny/shiny.css', 'preload_store.js', 'browser-update.js', 'embed.css', 'break_string.js', 'plugins.css', 'plugins_mobile.css']
+    config.assets.precompile += ['vendor.js', 'common.css', 'desktop.css', 'mobile.css', 'admin.js', 'admin.css', 'shiny/shiny.css', 'preload_store.js', 'browser-update.js', 'embed.css', 'break_string.js']
 
     # Precompile all defer
     Dir.glob("#{config.root}/app/assets/javascripts/defer/*.js").each do |file|

config/initializers/sprockets.rb

@@ -0,0 +1,4 @@
+require_dependency 'discourse_sass_importer'
+
+Sprockets.send(:remove_const, :SassImporter)
+Sprockets::SassImporter = DiscourseSassImporter

lib/discourse_sass_importer.rb

@@ -4,7 +4,11 @@
 class DiscourseSassImporter < Sass::Importers::Filesystem
   GLOB = /\*|\[.+\]/
 
+  # Depending upon where this is passed we might either be passed a string as the
+  # first argument or a sprockets context. If the first argument is a sprockets
+  # context we store it and use it to mark dependencies.
   def initialize(*args)
+    @context = args.first unless args.first.is_a? String
     @root = Rails.root.join('app', 'assets', 'stylesheets').to_s
     @same_name_warnings = Set.new
   end
@@ -31,7 +35,20 @@ class DiscourseSassImporter < Sass::Importers::Filesystem
   end
 
   def find(name, options)
-    if name =~ GLOB
+    if name == "plugins" || name == "plugins_mobile"
+      if name == "plugins"
+        stylesheets = DiscoursePluginRegistry.stylesheets
+      elsif name == "plugins_mobile"
+        stylesheets = DiscoursePluginRegistry.mobile_stylesheets
+      end
+      contents = ""
+      stylesheets.each {|css| contents << File.read(css) }
+      Sass::Engine.new(contents, options.merge(
+        filename: "#{name}.scss",
+        importer: self,
+        syntax: :scss
+      ))
+    elsif name =~ GLOB
       nil # globs must be relative
     else
       engine_from_path(name, root, options)
@@ -48,6 +65,7 @@ class DiscourseSassImporter < Sass::Importers::Filesystem
   def glob_imports(glob, base_pathname, options)
     contents = ""
     each_globbed_file(glob, base_pathname.dirname, options) do |filename|
+      depend_on(filename)
       unless File.directory?(filename)
         contents << "@import #{Pathname.new(filename).relative_path_from(base_pathname.dirname).to_s.inspect};\n"
       end
@@ -62,6 +80,21 @@ class DiscourseSassImporter < Sass::Importers::Filesystem
 
   private
 
+    def depend_on(filename)
+      if @context
+        @context.depend_on(filename)
+        @context.depend_on(globbed_file_parent(filename))
+      end
+    end
+
+    def globbed_file_parent(filename)
+      if File.directory?(filename)
+        File.expand_path('..', filename)
+      else
+        File.dirname(filename)
+      end
+    end
+
     def engine_from_path(name, dir, options)
       full_filename, syntax = Sass::Util.destructure(find_real_file(dir, name, options))
       return unless full_filename && File.readable?(full_filename)

lib/tasks/assets.rake

@@ -16,9 +16,6 @@ task 'assets:precompile:before' do
   # let's make precompile faster using redis magic
   require 'sprockets'
   require 'digest/sha1'
-  require_dependency 'discourse_sass_importer'
-
-  Sprockets::SassImporter = DiscourseSassImporter
 
   module ::Sprockets
 

spec/models/site_customization_spec.rb

@@ -163,30 +163,12 @@ describe SiteCustomization do
       c.mobile_stylesheet_baked.should == "#a{color:#000}\n"
     end
 
-    pending 'should allow including discourse styles' do
+    it 'should allow including discourse styles' do
       c = SiteCustomization.create!(user_id: user.id, name: "test", stylesheet: '@import "desktop";', mobile_stylesheet: '@import "mobile";')
       c.stylesheet_baked.should_not =~ /Syntax error/
       c.stylesheet_baked.length.should > 1000
       c.mobile_stylesheet_baked.should_not =~ /Syntax error/
       c.mobile_stylesheet_baked.length.should > 1000
-
-      # Vikhyat, this is giving me an anurism
-      #
-      # SiteCustomisation is not initializing sprockets and sprockets enviroment, they are required for asset helpers to function properly
-      # in travis we see this failure quite often:
-      #
-      # /home/travis/.rvm/gems/ruby-2.1.1/gems/sprockets-2.11.0/lib/sprockets/sass_functions.rb:63:in `sprockets_context'
-      # /home/travis/.rvm/gems/ruby-2.1.1/gems/sass-rails-4.0.2/lib/sass/rails/helpers.rb:23:in `asset_url'
-      # /home/travis/.rvm/gems/ruby-2.1.1/gems/sass-3.2.16/lib/sass/script/funcall.rb:113:in `_perform'
-      # /home/travis/.rvm/gems/ruby-2.1.1/gems/sass-3.2.16/lib/sass/script/node.rb:40:in `perform'
-      # eg: https://travis-ci.org/discourse/discourse/jobs/22413830
-      #
-      # This makes sense cause our compile_stylesheet method does not specify sprockets: { context: ctx, environment: ctx.environment }
-      # Thing is, where do you even get this magic context object from and how do you generate it, perhaps Rails.application.assets.context_class.new ...
-      # I dunno, its all voodoo to me
-      #
-      # The "sometimes" failing thing also needs to be determined, this should either always fail or never fail, its a strong indicator that a bunch
-      # of caching is happening, please resolve and re-enable the test
     end
 
     it 'should provide an awesome error on failure' do