[Major Changes][Sprockets 3 for LESS] Using versionized classes #5

bogdanRada · 2016-08-25T09:36:12Z

Hello.
I recently added support for Sprockets 3 for sprockets-sass gem ( here: petebrowne/sprockets-sass#38 ) and here petebrowne/sprockets-sass#39

After some discussion in the first pull request, we decided to use versionized classes, so that would be easier to maintain compatibility with other sprockets versions, so this uses same structure,

Basically the V2 classes will be used for sprockets 2.x
V3 ...for Sprockets 3.x
and V4...for Sprockets 4.x

The Registration class will automatically require the classes based on the version of sprockets loaded.

This pull request will also DROP Ruby 1.9 and also DROP TILT DEPENDENCY*

This pull request fixes some issues:

The importer can now import from globbed files and from deeply nested files
And now LESS files can be cached, using a custom cache store.

All tests are now passing for both 2.x and 3.x , except 6 tests related to the helpers we have for assets ( Which are broken because i honestly haven';t found a way to pass arguments to this methods, besides the first argument )

 body { background: asset-path("image.jpg", @prefix: '/themes'); }

This seems to thow "Unrecognized input" when trying to parse it with the Less::Parser .
I am going to still try and fix this, but i think this is not available for our less.js version (1.6.1) which is loaded by `less.rb`` (version 2.6.0 -- the last version)

There is though LESS.js version 1.7.1, which has support for this, but the less.js repository hasn't been updated though :(

Also there are some changes done for Sprockets 4.x but that is not yet complete. It does not fully support it yet.

But wanted to get some feedback on this, and maybe we can release the changes for sprockets 3.x and fix those other issues in other pull requests.

Let me know what you think. Have a nice day :)

P.S. If you have any idea how to pass those arguments to those helpers i would really appreciate.

lloeki · 2016-08-29T20:36:05Z

Thank you for this, I'll try to look into it soon!

bogdanRada · 2016-08-30T10:36:44Z

Thank You too. I have an idea how to fix the issue with the helpers ...but i am going to be out of the country for the next 3 weeks. Will continue working on that after i return. Thank You for the response and i can't wait to hear more from You :)

bogdanRada · 2016-09-19T15:29:50Z

I added few more commits but rebased again and squashed all commits. I fixed the issue with the helpers, now all tests are passing for sprockets 2.x and 3.x . There is still some work left to be done for sprockets 4.x but that can be done in another pull request.

Please review again this pull request and let me know what you think :) Thank you very much

bogdanRada · 2016-09-19T16:37:20Z

The tests are passing locally, on Travis they fail because travis somehow uses an older bundler version. Left a comment on this issue travis-ci/travis-ci#5831 . Hopefully someone will find a solution for travis.

bogdanRada · 2016-09-28T08:52:48Z

@lloeki , can you please review this pull request again? I fixed some issues with the importer and also the function parsing. I removed also ruby 2.0 and 2.1.5 from Travis YML file since 2.0 is not maintained anymore and 2.1.5 causes issues with bundler (because Travis has a older version of travis on server)

I also tested this implementation with sprockets 3.x with a real application and works fine .

Let me know what you think. Thank you very much.

lloeki · 2016-09-28T08:57:20Z

I was seeing your commits pouring in, so waited for you to ping me back that you were done :-) I'll look into it soon.

bogdanRada · 2016-09-28T08:58:10Z

Thanks :) , i fixed a minor issue . Replaced "compact.flatten" with "flatten.compact" in lib/sprockets/less/functions.rb. Checked other places. Should be good to go now :)

Support for Sprockets 4.x is not ready yet, but will continue on that on another pull request.

bogdanRada · 2016-11-14T21:34:08Z

Hello. I am just curios :) @loeki did you had any chance to look at this pull request? :)

bogdanRada · 2016-12-14T04:40:17Z

The issue with the helpers was fixed. Had to hack it a bit though...but it works.

Let me know if something needs to Be changed or refactored or something...Thanks a lot :)

lloeki · 2016-12-14T07:10:35Z

Wow it'd been so long since I've promised looking at this, sorry! Thanks a lot for your continued effort on this. I have a couple things to do but I'll be looking at it afterwards.

bogdanRada · 2017-05-15T05:58:12Z

It's been 5 months since last reply. Any chance of this getting reviewed? Please .

lloeki · 2017-05-15T06:06:44Z

Sorry, I'll have a look at it today.

lloeki · 2017-05-15T11:25:26Z

.travis.yml

-  - 2.1.0
+- 2.2.2
+- 2.2.3
+- 2.3.1


Can we have 2.4 here too?

added ruby 2.4.1 to travis

lloeki · 2017-05-15T11:26:25Z

.travis.yml

-  - 2.1.0
+- 2.2.2
+- 2.2.3
+- 2.3.1


Also, can those be specified as major.minor so that we get the latest patch automatically? I can't recall if Travis allows this.

i don't think Travis has that ability .

lloeki · 2017-05-15T11:30:21Z

sprockets-less.gemspec

+
+  # This is needed so that on rubygems.org we can see the actual date
+  # of the published version due to changes in Rubygems repo over the last year(2015 - 2016)
+  s.date = Date.today


I have an issue here with ruby 2.4 (ruby 2.3 plows through fine):

[!] There was an error parsing `Gemfile`: [!] There was an error while loading `sprockets-less.gemspec`: uninitialized constant Date

That is from bundler actually and i fixed it now.

lloeki · 2017-05-15T11:32:22Z

lib/sprockets/less.rb

+    # For Sprockets 2.x , although the file and the module name exist,
+    # they can't be used because it will give errors about undefined methods, because this is included only on Sprockets::Base
+    # and in order to use them we would have to subclass it and define methods to expire cache and other methods for registration ,
+    # which are not needed since Sprockets already  knows about that using the environment instead internally


Extra space before know. Also, can you hard-wrap this comment to 79?

lloeki · 2017-05-15T11:33:55Z

lib/sprockets/less/functions.rb

+::Less::Parser.class_eval do
+  attr_reader :tree
+
+


Remove that extra line.

lloeki · 2017-05-15T11:34:47Z

lib/sprockets/less/functions.rb

    end

+    return hash


Remove return keyword.

lloeki · 2017-05-15T11:35:29Z

lib/sprockets/less/functions.rb

+       arr << { match: m, index: offset } unless m.nil?
+       str = str[(offset + 1)..-1]
+   end
+   arr.uniq {|hash| hash[:match] }


Add blank line before implicit return value

Also, missing space after {

lloeki · 2017-05-15T12:32:58Z

lib/sprockets/less/v2/less_template.rb

+          default_less_config[:load_paths] = default_less_config[:load_paths].concat(default_less_config[:paths]) if default_less_config[:paths].is_a?(Array)
+          less =  default_less_config
+          less = merge_less_options(less.dup, @less_config) if defined?(@less_config) && @less_config.is_a?(Hash)
+          less


Add blank line before implicit return value

lloeki · 2017-05-15T12:36:43Z

lib/sprockets/less/v2/less_template.rb

+          options[:load_paths] = options[:load_paths].is_a?(Array) ? options[:load_paths] : []
+
+          if (load_paths = options[:paths]) && (other_paths = other_options[:paths])
+            options[:load_paths] = options[:load_paths]+ other_paths + load_paths


Missing space around +

lloeki · 2017-05-15T12:39:18Z

lib/sprockets/less/v2/less_template.rb

+          fetch_importer_class.fetch_all_dependencies(data, filename, less_options, css_options)
+        end
+
+        def merge_less_options(options, other_options)


So this is side-effectful on both options and other_options? I'd have expected only options to suffer from side effects (like Hash#merge!). Maybe some dup is in order, or explain the rationale and/or behaviour in a comment.

lloeki · 2017-05-15T12:39:28Z

lib/sprockets/less/v2/less_template.rb

+            options[:load_paths] = options[:load_paths]+ other_paths + load_paths
+          end
+          options[:load_paths] = options[:load_paths].concat(context.environment.paths)
+          options


Remove extra spaceAdd blank line before implicit return value

lloeki · 2017-05-15T12:40:05Z

lib/sprockets/less/v2/less_template.rb

+
+        def css_options
+          css_keys = [:compress, :optimization, :silent, :color]
+          Hash[less_options.to_enum.to_a.select{|k, _| css_keys.include? k}]


Add blank line before implicit return value

spaces all around block's { and }

lloeki · 2017-05-15T12:41:04Z

lib/sprockets/less/v2/less_template.rb

+          else
+            css
+          end
+


Remove blank line

lloeki · 2017-05-15T12:42:12Z

lib/sprockets/less/v2/less_template.rb

+            data = Sprockets::Less::Utils.read_file_binary(filename, options)
+            new_data, dependencies = process_dependencies(data)
+
+            css  = less_engine(new_data, less_options, css_options)


Remove extra space before =

lloeki · 2017-05-15T12:46:23Z

lib/sprockets/less/v2/less_template.rb

+            end
+          else
+            css
+          end


Line 135 exit point is easily overlooked due to too much nesting. Turn if css.nil? and if context.respond_to?(:metadata) into guard clauses that return css.

lloeki · 2017-05-15T12:46:38Z

lib/sprockets/less/v2/less_template.rb

+          run
+        end
+
+


Remove extra blank line

lloeki · 2017-05-15T12:46:55Z

lib/sprockets/less/v2/less_template.rb

+          @filename = input[:filename]
+          @source   = input[:data]
+          @context  = input[:environment].context_class.new(input)
+          run


Add blank line before implicit return value

lloeki · 2017-05-15T12:47:57Z

lib/sprockets/less/v2/importer.rb

+          engines = get_engines_from_attributes(context, attributes)
+          preprocessors = get_context_preprocessors(context, content_type)
+          additional_transformers = get_context_transformers(context, content_type, path)
+          additional_transformers.reverse + preprocessors + engines.reverse


Add blank line before implicit return value

lloeki · 2017-05-15T12:48:06Z

lib/sprockets/less/v2/importer.rb

+        def content_type_of_path(context, path)
+          attributes = context.environment.attributes_for(path)
+          content_type = attributes.content_type
+          [content_type, attributes]


Add blank line before implicit return value

lloeki · 2017-05-15T12:48:16Z

lib/sprockets/less/v2/importer.rb

+        def filtered_processor_classes
+          classes = [Sprockets::Less::Utils.get_class_by_version('LessTemplate')]
+          classes << Tilt::LessTemplate if defined?(Tilt::LessTemplate)
+          classes


Add blank line before implicit return value

lloeki · 2017-05-15T12:48:22Z

lib/sprockets/less/v2/importer.rb

+        # Returns nil if the path is already to a partial.
+        def partialize_path(path)
+          return unless path.basename.to_s !~ /\A_/
+          Pathname.new path.to_s.sub(/([^\/]+)\Z/, '_\1')


Add blank line before implicit return value

lloeki · 2017-05-15T12:48:29Z

lib/sprockets/less/v2/importer.rb

+            relative_path = base_path.relative_path_from(root_path).join path
+            paths.unshift(relative_path, partialize_path(relative_path))
+          end
+          [paths, root_path]


Add blank line before implicit return value

lloeki · 2017-05-15T12:48:46Z

lib/sprockets/less/v2/importer.rb

+
+          # Find base_path's root
+          paths, root_path = add_root_to_possible_files(context, base_path, path, paths)
+          [paths.compact, root_path]


Add blank line before implicit return value

lloeki · 2017-05-15T12:50:18Z

lib/sprockets/less/v2/importer.rb

+              return found if context.asset_requirable?(found)
+            end
+          end
+          nil


Use Enumerable#find instead of each

bogdanRada · 2017-05-15T12:51:51Z

Travis currently is failing , there seem to be some kind of caching problem, i am still trying to figure that out. Seems strange though, for other repositories i use same configuration, but for this repository it doesn't want to work . Need to spend more time investigating this problem

lloeki · 2017-05-15T12:52:53Z

lib/sprockets/less/v2/importer.rb

+          context = less_options[:custom][:sprockets_context]
+          (pathname = resolve(context, path, base_path))  || (return nil)
+          context.depend_on pathname
+          dependencies << pathname


Add blank line after to make the structure more clear

lloeki · 2017-05-15T12:53:07Z

lib/sprockets/less/v2/importer.rb

+              css = find_relative(import_path, pathname, less_options, css_options)
+              data_to_parse.gsub!(/^\s*@import\s*['"]#{Regexp.escape(import_path)}['"]\s*;/, css)
+            end
+          end


Add blank line after to make the structure more clear

lloeki · 2017-05-15T12:53:25Z

lib/sprockets/less/v2/importer.rb

+          final_css
+        end
+
+


Remove extra blank line

lloeki · 2017-05-15T12:53:44Z

lib/sprockets/less/v2/functions.rb

+
+        def font_path(source, options = {})
+          if options[:from_url] || (options.is_a?(Hash) && options.keys.size > 0)
+           quote(sprockets_context.font_path(source, map_options(options)))


Indent by 2

lloeki · 2017-05-15T12:54:06Z

lib/sprockets/less/v2/cache_store.rb

+          @environment = environment
+        end
+
+


Remove extra blank line

lloeki · 2017-05-15T12:54:24Z

lib/sprockets/less/v2/cache_store.rb

+          return if obj[:version] != version || obj[:sha] != sha
+          obj[:obj]
+        end
+


Remove blank line between end

lloeki · 2017-05-15T12:54:52Z

lib/sprockets/less/v2/cache_store.rb

+          obj = environment.send(:cache_get, "less/#{key}")
+          return unless obj.is_a?(Hash)
+          return if obj[:version] != version || obj[:sha] != sha
+          obj[:obj]


Add blank line before implicit return value

lloeki · 2017-05-15T13:04:26Z

Wow that was one hell of a review, nice piece of work, thanks a lot! I didn't find much questionable things :D Mostly that's about some style issues.

So let me know if you can find anything on the Travis front, fix the nitpicks and we can finally get this merged!

lloeki · 2017-05-15T13:06:03Z

lib/sprockets/less/v3/importer.rb

+          preprocessors = get_context_preprocessors(context, content_type)
+          additional_transformers = get_context_transformers(context, content_type, path)
+          postprocessors = get_context_postprocessors(context, content_type)
+          engines.reverse + preprocessors + additional_transformers.reverse  + postprocessors


Add blank line before implicit return value

lloeki · 2017-05-15T13:06:21Z

lib/sprockets/less/v3/importer.rb

+          content_type, attributes = content_type_of_path(context, path)
+          processors = get_all_processors_for_evaluate(context, content_type, attributes, path)
+          filter_all_processors(processors)
+          evaluate_path_from_context(context, path, processors)


Add blank line before implicit return value

lloeki · 2017-05-15T13:06:38Z

lib/sprockets/less/v3/importer.rb

+        def get_context_transformers(context, content_type, path)
+          available_transformers =  context.environment.transformers[content_type]
+          additional_transformers = available_transformers.key?(syntax_mime_type) ? available_transformers[syntax_mime_type] : []
+          additional_transformers.is_a?(Array) ? additional_transformers : [additional_transformers]


Add blank line before implicit return value

lloeki · 2017-05-15T14:08:01Z

sprockets-less.gemspec

  s.add_development_dependency 'test_construct',    '~> 2.0'
-  s.add_development_dependency 'rake'
+  s.add_development_dependency 'appraisal', '~> 2.1', '>= 2.1'
+  s.add_development_dependency 'rake', '>= 10.5', '>= 10.5'
 end


Add rubocop as a version-softlocked development dependency:

s.add_development_dependency 'rubocop', '~> 0.46.0'

bogdanRada force-pushed the feature/sprockets-3 branch from 051234a to 27043b1 Compare September 19, 2016 15:28

bogdanRada force-pushed the feature/sprockets-3 branch from caca5a9 to b295389 Compare September 20, 2016 07:27

bogdanRada mentioned this pull request Sep 20, 2016

Less support in sprockets >= 3.x for SCSS and SASS files. petebrowne/sprockets-sass#40

Closed

bogdanRada force-pushed the feature/sprockets-3 branch from 8f8be96 to 62733ac Compare September 20, 2016 08:02

bogdanRada force-pushed the feature/sprockets-3 branch from 656f969 to 90f77cc Compare September 28, 2016 08:49

bogdanRada force-pushed the feature/sprockets-3 branch 2 times, most recently from 5254e6f to 4af6a32 Compare September 28, 2016 09:04

sprockets 3 support

d10018e

bogdanRada force-pushed the feature/sprockets-3 branch from 2135777 to d10018e Compare September 28, 2016 09:09

lloeki reviewed May 15, 2017

View reviewed changes

add ruby 2.4.1 to travis

7bdf891

lloeki reviewed May 15, 2017

View reviewed changes

lib/sprockets/less/functions.rb Outdated

::Less::Parser.class_eval do

attr_reader :tree

Copy link

Owner

lloeki May 15, 2017

Choose a reason for hiding this comment

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

Remove that extra line.

fix bundler removing date

94048e7

lloeki reviewed May 15, 2017

View reviewed changes

lib/sprockets/less/functions.rb Outdated

end

return hash

Copy link

Owner

lloeki May 15, 2017

Choose a reason for hiding this comment

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

Remove return keyword.

lloeki reviewed May 15, 2017

View reviewed changes

lib/sprockets/less/v2/less_template.rb

else

css

end

Copy link

Owner

lloeki May 15, 2017

Choose a reason for hiding this comment

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

Remove blank line

lloeki reviewed May 15, 2017

View reviewed changes

lib/sprockets/less/v2/less_template.rb

run

end

Copy link

Owner

lloeki May 15, 2017

Choose a reason for hiding this comment

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

Remove extra blank line

lloeki reviewed May 15, 2017

View reviewed changes

lib/sprockets/less/v2/importer.rb

final_css

end

Copy link

Owner

lloeki May 15, 2017

Choose a reason for hiding this comment

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

Remove extra blank line

lloeki reviewed May 15, 2017

View reviewed changes

lib/sprockets/less/v2/cache_store.rb

@environment = environment

end

Copy link

Owner

lloeki May 15, 2017

Choose a reason for hiding this comment

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

Remove extra blank line

lloeki reviewed May 15, 2017

View reviewed changes

[Major Changes][Sprockets 3 for LESS] Using versionized classes #5

Are you sure you want to change the base?

[Major Changes][Sprockets 3 for LESS] Using versionized classes #5

Conversation

bogdanRada commented Aug 25, 2016 • edited

lloeki commented Aug 29, 2016

bogdanRada commented Aug 30, 2016

bogdanRada commented Sep 19, 2016

bogdanRada commented Sep 19, 2016

bogdanRada commented Sep 28, 2016 • edited

lloeki commented Sep 28, 2016

bogdanRada commented Sep 28, 2016 • edited

bogdanRada commented Nov 14, 2016

bogdanRada commented Dec 14, 2016

lloeki commented Dec 14, 2016

bogdanRada commented May 15, 2017

lloeki commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lloeki May 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lloeki May 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanRada commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lloeki commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanRada commented Aug 25, 2016 •

edited

bogdanRada commented Sep 28, 2016 •

edited

bogdanRada commented Sep 28, 2016 •

edited

lloeki May 15, 2017 •

edited

lloeki May 15, 2017 •

edited