Doesn't like javascripts with dashes

[jodigiordano]

With this config:

assets do
  serve '/js', from: '../client/scripts'

  js  :all, %w[ /js/jquery-1.8.3.js ]
end

AssetPack tries to find a file named jquery.js.

[j15e]

Could you provide more details? How do you know it looks for jquery.js?

[lukesmith]

I don't have an issue with dashes in a test case, but files which contain numbers following a . are causing problems, such as jquery.myplugin.1.8.0.js. The 0 gets treated as the cache buster and removed.

I've got two commits with tests covering a couple of cases including the failing cases https://github.com/lukesmith/sinatra-assetpack/commit/62de69ce158e63f31e2afb849713f2875c4b323b https://github.com/lukesmith/sinatra-assetpack/commit/82e122170487f936c0091d6881774d6463bb13f5.

[j15e]

Thanks for the update, I think the final solutions for this is using MD5 cache busters instead of time numbers. The work is almost done on feature/md5 branch but I have an issue with tests on travis-ci (works on my computer, but on travis-ci I get an error and I haven't found why yet) so I can't merge yet.

[blakeharv]

Man, I'm sooooo ready for MD5!

Is it this one? "ArgumentError: comparison of String with nil failed"

[lukesmith]

I get the same error as on travis-ci on the feature/md5 branch

1) Error:
test_non_existent_files_in_js_helper(NonExistentTest):
ArgumentError: comparison of String with nil failed

I can stop the exception occurring filtering out nil elements in the following method in buster_helpers.rb

def cache_buster_hash(*files)
  # files sometimes contains nil elements, which causes the ArgumentError above.
  # I haven't traced where the nil is coming from
  content = files.reject { |f| f.nil? }.sort.map { |f| File.read(f).to_s if f.is_a?(String) && File.file?(f) }.compact
  ...
end

The following test still fails.

  test "non-existent files in js helper" do
    get '/'
    assert body.include?('combine.js')
  end

the body contains

<script src='/script.min.061e5bc7462af86fdd2abc7f0b73eec7.js'></script>

[j15e]

Thanks for finding this one out! I think idealy we should trip the nil values before calling cache_buster_hash as it should be expected to receive nil values.

For the other failing test combine.js, I have no idea yet.

[j15e]

The bug is caused by non-existent files (aka files served by the app for exemple, not on the filesystem). I think they should have an MD5 digest too, or we are ignoring them by rejecting nil here. Not sure why it worked before with mtime_for as the logic is pretty much the same.

[lukesmith]

I think the test that's failing is wrongly asserting. My understanding is that combine.js wouldn't be included in the body in the production environment because it would have been merged into script.min.js. The test beneath this one tests the development environment and does successfully assert combine.js is in the body.

[j15e]

I think the concept is that non-existent files should always be included apart because they are "dynamic" files. The app test in this specific case define a get path for the combine.js file for exemple :

    get '/js/combine.js' do
      "alert('Spin spin sugar');"
    end

[j15e]

We could change this behavior as I think this is a bit of a weird behavior (if you don't want this file to be squash & compile, don't use assetpack...) but we should make sure this doesn't create new unpexpected behavior. We should verify the minified file includes alert('Spin spin sugar'); in both modes (prod/dev).

Also, I am not really sure what the last test non-existant asset hosts is about eheh