rstacruz / sinatra-assetpack

Package your assets transparently in Sinatra.
http://ricostacruz.com/sinatra-assetpack/
MIT License
541 stars 97 forks source link

enable dynamic asset caching as an option to speed up development reques... #94

Closed bporterfield closed 11 years ago

bporterfield commented 11 years ago

...ts - only compile dynamic assets after first request if the file has been modified.

In development, dynamic asset caching for large packages was slowing down page requests significantly. This flag enables caching for compiled assets until the file has been changed.

j15e commented 11 years ago

Hi, thanks for the PR, nice work, I have 2 comments :

bporterfield commented 11 years ago

Thanks for feedback. Renamed setting to simpler name and updated test!

j15e commented 11 years ago

I think instead of overwriting the File.send method manually you could use a stub and/or expect call from mocha as in this test :

https://github.com/rstacruz/sinatra-assetpack/blob/master/test/cache_test.rb#L23

http://gofreerange.com/mocha/docs/

bporterfield commented 11 years ago

Never used stubs but made an attempt. Thanks for feedback!

j15e commented 11 years ago

I update the test before merging I think you might like to have a look! Your test with raises was testing that something would not happen, I prefer to test that call happen but a limited times :

  test "served from cache" do
    app.assets.cache_dynamic_assets = true
    Sinatra::AssetPack::Css.stubs(:preproc).times(1)
    get '/css/screen.css'
    get '/css/screen.css'
    get '/css/screen.css'
    assert last_response.status == 200
  end

https://github.com/rstacruz/sinatra-assetpack/blob/master/test/cache_dynamic_assets_test.rb

bporterfield commented 11 years ago

Interesting! I was under the mistaken impression that times(x) would revert back to original method signature after x times. This makes a lot more sense. Thanks for the merge!