assets:clean / webpacker:clean task deletes the latest version of files when asset_host is set

[robotfelix]

Ruby version: 2.7.6
Rails version: 6.1.6
Webpacker version: 6.4.1

Expected behavior:

As documented by a comment in the implementation file itself, running the webpacker:clean task (or the enhanced assets:clean task) should always keep the latest version of compiled assets as indicated by manifest.json.

There is no expectation that setting config.asset_host (or the longer name of config.action_controller.asset_host) in production.rb should change this behaviour.

Actual behavior:

If a host is set for config.asset_host, then the webpacker:clean task will sometimes remove even the latest version of assets, leading to 404s for those assets. I think this is down to the age of the asset file.

Steps to reproduce:

1. Set config.asset_host to any host in production.rb, eg config.asset_host = example.com
2. Run RAILS_ENV=production bundle exec rails assets:precompile
3. Wait 1 second
4. Run RAILS_ENV=production bundle exec rake webpacker:clean[2,1]
5. See that all your new assets just got removed!

(To save you from looking, the '1' argument in the command sets the maximum age of assets that are no longer considered the "latest" version to 1 second.)

Cause:

Setting config.asset_host causes full URLs to be inserted into the manifest.json, eg:

{
  // ... other assets ...
  "application.js": "//example.com/packs/js/application-50abb05db229c31c1e24.js",
  // ... other assets ...
}

which is already documented in the troubleshooting docs relating to another issue.

I couldn't locate the PR/motivation for this change to manifest.json, but as a reminder, prior to shakapacker/webpacker v6 manifest.json did not include the asset_host.

Unfortunately the implementation of webpacker:clean was not updated to handle this, so it is expecting to get local file paths but getting remote URLs from manifest.json. Since none of the local assets paths match the remote URLs, this in turns means that no local assets are kept due to being the current version, and they are only ever kept if they meet the criteria for keeping a backup.

shakapacker/lib/webpacker/commands.rb

Lines 69 to 78 in 3ba9970

	def current_version
	packs = manifest.refresh.values.map do |value|
	value = value["src"] if value.is_a?(Hash)
	next unless value.is_a?(String)
	File.join(config.root_path, "public", "#{value}*")
	end.compact
	Dir.glob(packs).uniq
	end

In particular, on line 74 value is a URL so sticking this on the end of the project root path produces a half-local-path half-URL string.

Workaround:

Like the linked troubleshooting issue, setting the WEBPACKER_ASSET_HOST env variable to an empty string causes the manifest.json to revert to having relative paths, so the webpacker:clean task then works correctly.

Thoughts on priority:

This is quite a nasty issue as things might work absolutely fine when initially upgrading to shakapacker, but then start failing in weird ways on subsequent builds.

You can get very strange behaviours since all of the logic is based off of the file age, so in my case running assets:clean removed a foo.js file while leaving the equivalent foo.js.gz file intact, causing much head scratching due things working in some browsers and not others.

[tomdracz]

Thanks for raising. I was surprised by:

I couldn't locate the PR/motivation for this change to manifest.json, but as a reminder, prior to shakapacker/webpacker v6 manifest.json did not include the asset_host.

But it might have been caused by rails/webpacker#2802

https://github.com/shakacode/shakapacker/blob/master/package/environments/base.js#L66 this line used to pass in path without CDN b041a1d

rails/webpacker#1974 - that might be the missing piece here but will need to dig into this further to figure out how it all ties together

[robotfelix]

It's probably not too difficult (famous last words) to update the implementation of current_version to trim off any asset_host (or WEBPACKER_ASSET_HOST if set) from the manifest values, which would fix this issue.

However since the change to manifest.json is the source of at least one other troubleshooting issue, it does seem like understanding the rationale behind the change would be useful!

[tomdracz]

Agree, trimming the host from clean task would be a quick solution, but I do want to trace the changes to figure out what's what. The change linked happened in unreleased Webpacker v6 so I'll just need to trace down the commits and figure out how it worked before, to mimic the same setup!

[Judahmeek]

@ahangarha this is actually a fairly serious issue.

[ahangarha]

This means we should be able to fix this issue at this part:

shakapacker/lib/webpacker/compiler.rb

Lines 97 to 99 in bb7cd6e

	env.merge("WEBPACKER_ASSET_HOST" => ENV.fetch("WEBPACKER_ASSET_HOST", ActionController::Base.helpers.compute_asset_host),
	"WEBPACKER_RELATIVE_URL_ROOT" => ENV.fetch("WEBPACKER_RELATIVE_URL_ROOT", ActionController::Base.relative_url_root),
	"WEBPACKER_CONFIG" => webpacker.config_path.to_s)

By setting WEBPACKER_ASSET_HOST (and in version 7 SHAKAPACKER_ASSET_HOST) to an empty string if it is not set.

@Judahmeek Am I right? Should I proceed with PR and testing the result?

[jschroe212]

For my application, forcing SHAKAPACKER_ASSET_HOST="" was a solution, but I believe that depends on the application. We only referenced WebPack image assets from within CSS files. Since the CSS file was included by javascript_pack_tag, it included the CDN hostname. The CSS file then had relative URLs resolved by Webpack and the browser requested them with the same CDN host, and everything worked as expected.

Now see the following comment from webpacker: rails/webpacker#1845 (comment)

If an application resolves assets like this and uses them in templates or some other similar manner in javascript at runtime, then with the way this was set up with webpack, SHAKAPACKER_ASSET_HOST would need to be set at compile time. Webpack output.publicPath gets set to include SHAKAPACKER_ASSET_HOST fixing the runtime JS URL resolves but webpack-assets-manifest.publicPath option defaults to true so the CDN is then also included in the manifest file which breaks the enhanced assets:clean.

@tomdracz brought up the following comment: https://github.com/rails/webpacker/pull/1974/files#diff-7af8667a3e36201db57c02b68dd8651883d7bfc00dc9653661be11cd31feeccd

Setting WebPackAssetsManifest.publicPath to the path withoutCDN makes more sense to me and would fix this issue. Is there a use-case for including the full CDN URLs in the asset manifest?

[jschroe212]

shakapacker/package/environments/base.js

Line 66 in cdf3283

publicPath: true

https://github.com/webdeveric/webpack-assets-manifest#publicpath

So I can't answer it without going deeper, but was including the CDN in the manifest file intended? If it was, and there's a usecase for it, then asset:clean needs to be updated to account for it. If it wasn't, then webpack-asset-manifest.publicPath simply needs a better default.