bjarosze / riot_js-rails

Muut Riot integration with Rails
28 stars 14 forks source link

Add support for sprockets version 2, 3 and 4 #18

Closed osamutake closed 7 years ago

osamutake commented 8 years ago

Thank you for developing such a valuable library.

With using it in various systems, I found the current implementation of sprockets_processor_v2.rb is not actually compatible with sprockets 2.x.

With rails 3.2.11 with sprockets 2.2.3, it emits undefined method error:

`method_missing': undefined method `assets' for #<Rails::Railtie::Configuration:0x00000000000000> (NoMethodError)

With rails 4.0.0 with sprockets 2.12.4, it does not give a correct URL for converted asset:

<script src="/javascripts/riot-tags/test.js.tag.js"></script>

expected result is

<script src="/assets/riot-tags/test.js"></script>

Moreover, current haml support is very much limited. It indeed works with sprockets 2.2 but not with 2.12, 3.x or 4.x.

I referred to the article "Supporting All Versions of Sprockets in Processors," and implemented a processor that supports all sprockets 2, 3 and 4 as this PR.

Supporting All Versions of Sprockets in Processors: https://github.com/rails/sprockets/blob/master/guides/extending_sprockets.md#supporting-all-versions-of-sprockets-in-processors

This code was tested on various conditions listed below.

ruby version rails version sprockets version result comment
1.9.3 3.0.20 -- NG sprockets is not available
1.9.3 3.1.12 2.0.5 ok extension must be .js.tag instead of .tag
1.9.3 3.2.11 2.2.3 ok extension must be .js.tag instead of .tag
1.9.3 4.0.0 2.12.4 ok extension must be .js.tag instead of .tag
2.2.1 4.0.13 2.12.4 ok extension must be .js.tag instead of .tag
1.9.3 4.1.16 2.12.4 ok extension must be .js.tag instead of .tag
2.2.1 4.2.0 3.7.0 ok
2.3.1 5.0.0.1 3.7.0 ok

Test scripts are available at: https://gist.github.com/osamutake/0ccbdf96a5ac70a36ebce38db0a33137

To be honest, I am not sure if all the lines in the definition of Processor are meaningful. Some part may be nonsense, i.e. never reached or referred to. But anyways, it works at least.

Your comments and critics are very much welcome. Have fun!

osamutake commented 8 years ago

slim support was added! Test code was added!

I refactored the registration of haml translator and newly added slim support.

To enable haml and/or slim translator, haml and/or slim gem must be included in Gemfile, respectively, while haml-rails and/or slim-rails are not required.

The test code is now included in the repository, which can be invoked by rake test:sprockets_versions.

The test result is as following.

result rails version sprockets version ruby version
SUCCESS 3.1.12 2.0.5 1.9.3p551
SUCCESS 3.1.12 2.0.5 2.0.0p648
SUCCESS 3.1.12 2.0.5 2.1.10p492
SUCCESS 3.1.12 2.0.5 2.2.5p319
SUCCESS 3.1.12 2.0.5 2.3.1p112
SUCCESS 3.2.11 2.2.3 1.9.3p551
SUCCESS 3.2.11 2.2.3 2.0.0p648
SUCCESS 3.2.11 2.2.3 2.1.10p492
SUCCESS 3.2.11 2.2.3 2.2.5p319
SUCCESS 3.2.11 2.2.3 2.3.1p112
SUCCESS 4.0.4 2.12.4 1.9.3p551
SUCCESS 4.0.4 2.12.4 2.0.0p648
SUCCESS 4.0.4 2.12.4 2.1.10p492
SUCCESS 4.0.4 2.12.4 2.2.5p319
SUCCESS 4.0.4 2.12.4 2.3.1p112
SUCCESS 4.0.13 3.7.0, 2.12.4 2.0.0p648
SUCCESS 4.0.13 3.7.0, 2.12.4 2.1.10p492
SUCCESS 4.0.13 3.7.0, 2.12.4 2.2.5p319
SUCCESS 4.0.13 3.7.0, 2.12.4 2.3.1p112
SUCCESS 4.1.16 3.7.0, 2.12.4 1.9.3p551
SUCCESS 4.1.16 3.7.0, 2.12.4 2.0.0p648
SUCCESS 4.1.16 3.7.0, 2.12.4 2.1.10p492
SUCCESS 4.1.16 3.7.0, 2.12.4 2.2.5p319
SUCCESS 4.1.16 3.7.0, 2.12.4 2.3.1p112
SUCCESS 4.2.7.1 3.7.0 1.9.3p551
SUCCESS 4.2.7.1 3.7.0 2.0.0p648
SUCCESS 4.2.7.1 3.7.0 2.1.10p492
SUCCESS 4.2.7.1 3.7.0 2.2.5p319
SUCCESS 4.2.7.1 3.7.0 2.3.1p112
SUCCESS 5.0.0.1 3.7.0 2.3.1p112
brauliobo commented 7 years ago

sorry for the big delay @osamutake, how is your availability to help maintain this gem?

brauliobo commented 7 years ago

also good to note on the readme about the slim support

brauliobo commented 7 years ago

isn't the slim pretty version required? https://github.com/slim-template/slim/issues/649#issuecomment-138089811

brauliobo commented 7 years ago

@osamutake this broke some things, I'll have to revert image

brauliobo commented 7 years ago

Also no error being raised when the tag file is invalid

osamutake commented 7 years ago

Could you please specify which tests failed under which environment? Since my code do not contain any reference to "image_path," I do not see what is the "some things."

In addition, could you please show an invalid tag file that formerly raised an error and does not after this commit with the specific testing environment? Both my code and the old code do not contain any effective 'raise' or 'rescue.' I'm curious why they behave differently.

brauliobo commented 7 years ago

image_path is a sprockets asset helper, apparently the way you did is changing the binding @osamutake

this is possibly related to the silented errors

osamutake commented 7 years ago

Thank you for your quick reply. I'll look into it.