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

Run image compress even if Rails.application.assets is nil #121

Closed
wants to merge 5 commits into from
Closed

Run image compress even if Rails.application.assets is nil #121

wants to merge 5 commits into from

Conversation

iggant
Copy link
Contributor

@iggant iggant commented Jan 15, 2016

Since rails/sprockets-rails@d7c7ee1#diff-b295f4f07d2e863f15e53eb2047cfd1f we can't rely on app.assets for register_preprocessor

When compile=false and we precompile assets
register_preprocessor?(app) method will always return false

Since rails/sprockets-rails@d7c7ee1#diff-b295f4f07d2e863f15e53eb2047cfd1f we can't rely on app.assets for register_preprocessor
@toy
Copy link
Owner

toy commented Jan 15, 2016

Thanks for contribution!
It would be much better if the differentiation of behaviour would be based on availability of methods instead of gem versions.
It seems that the check for presence of app.assets is not really needed as if options are set, but there are no assets, then the processor will not be used, so maybe it can be replaced with true without conditions.
Also #120 looks more concise; as calls to register_preprocessor are identical, it would be better to just find the object on which to call the method instead of duplicating code.

This class allow us to return result hash to skip gzip file generation
@iggant
Copy link
Contributor Author

iggant commented Jan 17, 2016

Rewrite conditions for register_preprocessor?

For Sprocket intoduce new class ImageOptimProcessor, because proc object will use LegacyProcProcessor - this doesn't support return hash to configure asset

by default Sprocket produce gzip file, if it has preprocessor for file, but this doesn't have sence because image is already optimized,

if this pull request will be accepted rails/sprockets#221 then sprocket will skip gzip file generation.

@iggant
Copy link
Contributor Author

iggant commented Jan 19, 2016

so we have:
due to this rails/sprockets#224 we agreed that if preprocessor retrun nil charset in result hash sprockets will not generate gzip file, and I rewrite image preprocessor

also rewrite code to fit your rubocop policies

@iggant
Copy link
Contributor Author

iggant commented Feb 2, 2016

This pull request rails/sprockets#228 was merged to sprockets so can count on this behaiviour.
do you have some rewrite request, probably change some code?

@toy
Copy link
Owner

toy commented Feb 21, 2016

Hey Anton, sorry for long wait with this.
I've finally looked into how to properly work with sprockets.
One thing I've noticed is that it would make more sense to register image_optim as a compressor (as described in Extending Sprockets#Compressors), but unfortunately it doesn't work as compressors are finally used only for js and css (rails/sprockets#246).
Another thing is that a proc can still be used, but the number of arguments is important (https://github.com/rails/sprockets/blob/3.x/lib/sprockets/processing.rb#L234).
I'm trying to build a fix in sprockets-rails-3 branch.

toy added a commit that referenced this pull request Feb 21, 2016
…ass processor to register_processor, resolves #120, resolves #121, resolves #126
toy added a commit that referenced this pull request Feb 21, 2016
…ass processor to register_processor, resolves #120, resolves #121, resolves #126
@toy toy closed this in 72e9c35 Feb 21, 2016
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.

2 participants