AssetSync/asset_sync

AssetSync is not uploading .sprockets-manifest-*.json with include_manifest = true

dlinch opened this issue · 8 comments

I am trying to get the AssetSync upload working. At one point, I did see this working about a week or two ago, but when I did some spelunking I'm not how it could work right now. I have a hunch Sprockets might have changed something under the hood.

Inside of storage.rb, the method: get_manifest_path is passed to always_upload_files, so that we can fetch the manifest file when the config option include_manifest is true.

Now, from what I can tell, get_manifest_path is reaching into the Sprockets manifest and pulling out the filename.

This is returning a full filepath for me though, and looks something like: /Users/dlinch/work/project-name/public/assets/.sprockets-manifest-7fb3728dde636cbfcd21324e2e0e7df2.json1

So when that file is checked inside the upload_files method, there is a check for:
File.file? "#{path}/#{f}", which will return false because every other file is a simple filename that it throws the relative path behind, and that will not work for this. I did check the Sprockets manifest generation in my local rails console, and filename is returning a full path there as well.

What's the expected value?
We can access rails paths like Rails.root and Rails.public_path
But I don't know what value should be returned from get_manifest_path
Plus I cannot put a test case about it in spec...

@PikachuEXE It seems like the expected value should just be the filename, but the filename method is returning the full file path, which is passed on directly. The later code seems like it's expecting just a filename as it's constructing the relative file path to check against to upload.

I fixed this issue locally with a monkeypatch, in my AssetSync initializer I reopen the class:

module AssetSync
  class Storage
    def get_manifest_path
        manifest = Sprockets::Manifest.new(ActionView::Base.assets_manifest.environment, ActionView::Base.assets_manifest.dir)
        manifest_path = manifest.filename.sub("#{manifest.directory}", AssetSync.config.prefix)
      [manifest_path.sub(/^#{path}\//, '')] # full path to relative path
    end
  end
end

I'd be more than happy to open a PR if you think this is legit like I do. It kind of feels like Sprockets is the issue here, I have no idea why you would return a full file path for a filename method, that makes no sense to me.

OK let me put my debugging result: (running in an actual rails project console in development)

manifest = Sprockets::Manifest.new(ActionView::Base.assets_manifest.environment, ActionView::Base.assets_manifest.dir)
manifest_path = manifest.filename
# => "/Users/pikachuexe/projects/spacious/spacious-rails/public/assets/.sprockets-manifest-f801a5c9ce179e5090d136d925924220.json"

path=::Rails.public_path
# => #<Pathname:/Users/pikachuexe/projects/spacious/spacious-rails/public>

manifest_path.sub(/^#{path}\//, "")
# => "assets/.sprockets-manifest-f801a5c9ce179e5090d136d925924220.json"

What do you have if you the above code and the code you modified?

Edit 1: got same result in prod

@PikachuEXE I see the issue. I set my public_path value in the initializer when I was experimenting with setting assets in a different folder. This was sending back './public' instead of "/Users/dlinch/work/project/public", and the sub wasn't working properly because it was a relative path instead of a full path.

Thanks for helping me get through this. It does beg the question as to whether the gem should be exposing that value in the config initializer, it only seems to allow you to shoot yourself in the foot with it. That or it should be documented in the ReadMe.

I think to make it work correctly we need absolute path all the time
Can you test how to get absolute path via a relative path?

I plan to update https://github.com/AssetSync/asset_sync/blob/master/lib/asset_sync/config.rb
to add public_path= to convert input value into an absolute path in Pathname

You could probably update public_path in config to something like:

@public_path.present? ? Rails.root.join(@public_path) : ::Rails.public_path

Maybe we can memoize it too?

Opened #397
Take a look and test when free?

@PikachuEXE I added a comment on the PR; I think it's a good start.