AssetSync / asset_sync

Synchronises Assets between Rails and S3
1.88k stars 346 forks source link

Respect explicitly configured manifest path for modern Rails/Sprockets #434

Closed krasnoukhov closed 1 year ago

krasnoukhov commented 1 year ago

In our Rails 6/7 apps that use Sprockets 3/4 we configure manifest path explicitly like so:

config.assets.manifest = Rails.root.join("config/manifest.json")

Because of that asset_sync does not work properly because it does not pick up proper manifest, since it's located out of assets directory. Here is relevant code in sprockets source:

https://github.com/rails/sprockets/blob/3.x/lib/sprockets/manifest.rb#L54-L63

So for us the solution was to explicitly path manifest path (which is nil unless defined explicitly) as a 3rd argument to Sprockets::Manifest.new which actually makes it work. This works for both Sprockets 3 and 4.

We've been running this patch for years now. Hopefully this makes sense and this contribution gets accepted. There are no specs for manifest-related code so I was only able to add a spec for manifest path.

Also, looks like manifest.yml is not a thing from Rails 4: https://github.com/rails/sprockets-rails#changes-from-rails-3x So maybe that is something to be cleaned up.

PikachuEXE commented 1 year ago

Looks fine merging But will wait for https://github.com/AssetSync/asset_sync/pull/435 before release (unless it's stuck

krasnoukhov commented 1 year ago

Thanks @PikachuEXE, sounds good

krasnoukhov commented 1 year ago

Looks like #435 is a bit stuck after all... Do you mind cutting a release @PikachuEXE? Thanks

PikachuEXE commented 1 year ago

Released in https://rubygems.org/gems/asset_sync/versions/2.18.1