Ortuna / padrino-pipeline

Sprockets for padrino apps
MIT License
21 stars 8 forks source link

Default assets paths don't work on Sinatra::AssetPack. It needs to be relative. #8

Closed hongliang-goudou closed 11 years ago

hongliang-goudou commented 11 years ago

The default assets paths are absolute:

@image_assets = "#{app_root}/assets/images"
@js_assets    = "#{app_root}/assets/javascripts"
@css_assets   = "#{app_root}/assets/stylesheets"

They are not compatible with Sinatra::AssetPack. The line 252 and line3 of sinatra-assetpack/lib/sinatra/assetpack/options.rb shows:

tuples = @served.map { |prefix, local_path|
          path = File.expand_path(File.join(@app.root, local_path))
          ...

Take js assets for example. @app.root is "/path/to/padrino/root/app", and local_path is just what we defined of @js_assets, which is "/path/to/padrino/root/app/assets/javascripts'. Pay attention to that join method calling. So path variable will be a mess: "/path/to/padrino/root/app/path/to/padrino/root/app/assets/javascripts". That stops AssetPack from working.

Ortuna commented 11 years ago

@hongliang-goudou Thanks for your contribution!

I see what you are saying, I will take a look at this further when I get a chance.

Two things come to mind.

Thanks again!

hongliang-goudou commented 11 years ago

Sorry I'm a newbie of ruby and I forgot to run the rake test. I modified my code to satisfy the test. Now it should be ok. Please check it again.

Another thing: I take a look at the test case and find out the reason why the original code passed the test is because it only tested http response status:

get '/assets/javascripts/application.js'
assert_equal 200, last_response.status

That's not enough. Although the sinatra-assetpack with wrong local_path configuration will return nothing when the "js" helper called, it still return 200 OK and output the file content. But I don't think it's a bug of assetpack. It's just serving the javascripts folder without any pipeline function because the pipeline local_path is configured wrong, not the serving path. Later I will pull another request.

Thank you!

hongliang-goudou commented 11 years ago

new testcases pushed. I only added one. The others needs to be added too. The purpose of added test case is to assert that sinatra-assetpack's helpers js/css/img should work normally.

Ortuna commented 11 years ago

@hongliang-goudou No problem, every one starts somewhere :smile: This looks good I'll review and merge it tonight!