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

Raising an error when an asset is not found #127

Closed
tiagobabo opened this issue Feb 24, 2023 · 4 comments
Closed

Raising an error when an asset is not found #127

tiagobabo opened this issue Feb 24, 2023 · 4 comments

Comments

@tiagobabo
Copy link

tiagobabo commented Feb 24, 2023

As per the Rails docs, if one is using sprockets-rails >= 3.2.0 it's possible to set config.assets.unknown_asset_fallback = false and raise an error when an asset is not found. Looking at the current implementation of asset_url we get a warning if propshaft is unable to resolve an asset URL. Would it be possible to have a similar configuration that would allow raising an error instead (especially on assets:precompile)?

@rmacklin
Copy link
Contributor

rmacklin commented Feb 24, 2023

It's worth noting that propshaft already does raise a MissingAssetError for assets that aren't resolved when the asset is referenced directly in an actionview helper method like javascript_include_tag, etc., as seen here:

def compute_asset_path(path, options = {})
Rails.application.assets.resolver.resolve(path) || raise(MissingAssetError.new(path))
end

The code linked in @tiagobabo's post is specific to assets that are referenced in a url(...) value within a CSS file, which are potentially transformed by the CssAssetUrls processor.

Note, however, that some expressions inside url() should never be transformed (e.g. an inline data-URI or a fully qualified URL to an external domain). This explains why the current implementation (of not raising an error) can be useful.

I do see the value in adding such a configuration to throw an exception rather than just logging a warning if an asset inside url() can't be resolved, but it seems like it would have to be restricted to only apply for expressions inside url() that makes sense to be transformed (e.g. relative URLs).

@tiagobabo
Copy link
Author

You're right, this only happens for assets required inside url(...), thanks for clarifying that.

@brenogazzola
Copy link
Collaborator

My original implementation was for a raise, but when I started migration my own app from sprockets to propshaft, I noticed that it was easier to let all errors to be logged and fix them in batches than doing the crash-fix-reload-crash-fix-reload cycle.

Is there a reason why raising would be better? I'm asking because between our two experiences we might figure out a third option that might be better than logging/raising

@brenogazzola
Copy link
Collaborator

I’ll close this issue for now as it’s not a bug but a feature request, and from my experience logging provides a better workflow than raising. If anyone has further input, feel free to comment.

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

No branches or pull requests

3 participants