Less support in sprockets >= 3.x for SCSS and SASS files. #40
Conversation
bogdanRada
changed the title
|
Travis Failed for ruby 2.0 and 2.1.5 because of an issue with bundler on Travis itself, it somehow requires an older version of bundler. I left a comment on this travis-ci/travis-ci#5831 for that. Tests are passing locally for all ruby versions. |
|
@petebrowne , can you please review this pull request? This also fixes a bug in the V3::Importer where :data key is deleted from context ( Which was a bug) Thank you very much |
| @@ -124,7 +124,7 @@ def call_processor_input(processor, context, input, processors) | |||
| def handle_complex_process_result(context, result, processors) | |||
| data = result[:data] if result.key?(:data) | |||
| context.metadata.merge!(result) | |||
| context.metadata.delete(:data) | |||
| # context.metadata.delete(:data) | |||
There was a problem hiding this comment.
Yes, this was a bug. when i made the importer i was looking at previous versions of sprockets, to be able to implement the "old" way of processing processors in a "new" way ( because the old way was removed without replacement) , and that line should be removed in order for the Importer to work correctly. Otherwise it causes issues with other engines because the data that is returned by those engines is not being merged in the context, which caused the Importer to show the original data from the processed file
This fixes this bug. Is a huge bug but the fix was minor. This is why i felt like this pull request needs to be reviewed ASAP.
There was a problem hiding this comment.
Since it's urgent we should separate this into another PR and I'll merge immediately while we discuss the Less processing
There was a problem hiding this comment.
ok. I will open a pull request for this today , a bit later , after i get some sleep. It's 2 AM here
| @@ -57,6 +57,12 @@ def get_class_by_version(class_name, version = version_of_sprockets) | |||
| nil | |||
| end | |||
|
|
|||
| def get_less_class_by_version(class_name, version = version_of_sprockets) | |||
| constantize("Sprockets::Less::V#{version}::#{class_name}") | |||
There was a problem hiding this comment.
Where are the Less classes defined?
There was a problem hiding this comment.
The Less classes are defined in sprocket-less gem, and this is needed for integration with that gem ( As i said in the description, this is the pull request for LESS with sprockets 3 lloeki/sprockets-less#5 )
I know this is a bit ugly , but if that gem is not required, this won't affect anything. And even if you require it but you don't have any less files, those importers from that gem will never be called and neither this code.
Only if you use LESS inside SASS or SCSS files ( or viceversa ), this code will execute.
I honestly found this when i wanted to integrate this gem in one of my applications, i realized i was using LESS inside SASS files ( by requiring them) and with sprockets 2.x that worked fine.
This add support for this type of usage for newer versions of sprockets too.
Hope now is much more clear. Let me know if you have any other questions. I will be very glad to help any way i can :)
There was a problem hiding this comment.
Maybe would be better if I check first if defined?(Sprockets::Less::VERSION) before trying to constantize the string and just return nil ( if not available) , which will be handled in the Importer , where we actually call try to add this class in the list of processors for that file.
What do you think ? I want to use the VERSION to make sure that the Sprockets::Less is not being defined from some other gem. or other place.
EDIT: You already answered this in the previous comment left on the Importer :) Which is better than my suggestion.
| @@ -194,6 +194,8 @@ def filter_all_processors(processors) | |||
| end | |||
|
|
|||
| def evaluate_path_from_context(context, path, processors) | |||
| less_processor = Sprockets::Sass::Utils.get_less_class_by_version('LessTemplate') | |||
There was a problem hiding this comment.
There's no need to get the class if we're not processing .less files
There was a problem hiding this comment.
We are not processing them, the Less Importer will process them, but we need to call it : )
Problem is the new way of registration of sprockets doesn't allow to fetch engines that are registered on a file extension (e.g. .less) , only if you check the content type ( text/css ) but Sprockets 3.x doesn't have anymore Less support so this content type is now not being used for less. Probablly i should find a better way of somehow link the content type of text/css to less but I haven't found yet that magic :(
So as a ugly hack , this is needed . Hope you understand my point..
Once i can find the magic of linking the content type text/css to the LESS preprocessor we can remove this 2 lines for LESS from here. But i am afraid i will need much more time to find this magic thing.
So since the integration with less is very small we can keep this, and once i can fix that thing with content type i will definitely open a new pull request to remove this ugly hack.
There was a problem hiding this comment.
Yeah I understand, what I mean is that we shouldn't even attempt to get the less processor if the path doesn't include .less, something like:
if path.include?('.less')
less_processor = Sprockets::Sass::Utils.get_less_class_by_version('LessTemplate')
processors = [less_processor] if less_processor
endIt might be premature optimization, but using Less inside Sass isn't standard, and we should try to limit the impact of this change for users who only use Sass
There was a problem hiding this comment.
yes . That is even better :) This will fix my concern about constantize that i said in the next comment :)
|
This can be closed in favor of this #41 which fixes this problem and we no longer need to add workarounds for less. |
Hello, i have recently added support for sprockets 3.x for Less too in this pull request lloeki/sprockets-less#5
I recently discovered that with Sprockets 2.x it is possible to use Less inside SCSS or SASS files.
( I mean requiring Less files or importing them inside SCSS or SASS files )
This adds this support to sprockets >= 3.x
Without this changes , trying to use LESS inside SASS or SCSS will result in a error.
I tested this and now is working ok. ALthough the pull request for Sprockets-Less is not yet merged into master, this won't affect anything, because if the class is not available will simply skip the step of
setting the correct processor for LESS.
I am not sure how to detect other processors for LESS but i will try to find a better way in maybe other pull request. Currently i am checking if the class exists.
Please let me know what you think