Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 29, 2023

Conversation

krasnoukhov
Copy link
Contributor

@krasnoukhov krasnoukhov commented Jul 28, 2023

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
Copy link
Member

Looks fine merging
But will wait for #435 before release (unless it's stuck

@PikachuEXE PikachuEXE merged commit 01206fc into AssetSync:master Jul 29, 2023
@krasnoukhov
Copy link
Contributor Author

Thanks @PikachuEXE, sounds good

@krasnoukhov
Copy link
Contributor Author

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

@PikachuEXE
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants