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

add ability to generate static gzip files #17

Merged
merged 4 commits into from Feb 28, 2013

Conversation

beanieboi
Copy link
Contributor

hey

i needed to generate static gzip files for nginx. here is the feature

thanks
ben

@ixti
Copy link
Contributor

ixti commented Feb 25, 2013

Thanks for your pull request. Can you please add tests?
Also I think there's no need to gzip JPEGs, so probably we can have configuration option as: list of mimetypes that are OK to be gzippable, true for default list of mimrtypes or false (default) for no gzipping at all.

@beanieboi
Copy link
Contributor Author

hey @ixti

i added some basic test for the configuration option. i couldn't find any test for the generated files. did i miss something? please point me to some!

i'm not sure if if we need a list of mimetypes. i never had this setup where you enabled static gzip only for a few assets, it was always an all or nothing. (YMMV)
do you have suggestions for such a list?
i theory you are right, maybe its not worth compressing JPEGs (i have some which are reduced by 5-8% in file size) but i would suggest that we add this feature later, if there is a demand for this?

@ixti
Copy link
Contributor

ixti commented Feb 25, 2013

Well, I would set default list to js + css only, but I tend to agree that most of the time it's all or nothing. And we can add it once somebody will hit other than this requirement - leaving true/false for now. Well I can add test by myself, or you can add it close to this one:

https://github.com/ixti/jekyll-assets/blob/master/spec/lib/jekyll/assets_plugin/site_patch_spec.rb#L103

Also, make gzip false by default please. And if you can a small description in config would be highly appreciated :D

@beanieboi
Copy link
Contributor Author

cool! i will add test and description!

@ixti
Copy link
Contributor

ixti commented Feb 25, 2013

Thanks! And then I'll publish fresh version!

@ixti
Copy link
Contributor

ixti commented Feb 26, 2013

Re what types to gzip. I think we will stick to the same logic as Sprockets does for it's manifest:
sstephenson/sprockets@eb84c41

In our case it would be something like:

class AssetFile
  # ...

  def write
    # ...
    @asset.write_to dest_path
    @asset.write_to "#{dest_path}.gz" if zip?
    # ...
  end

  def zip?
    @site.assets_config.gzip and @asset.is_a? Sprockets::BundledAsset
  end

  # ...
end

@ixti
Copy link
Contributor

ixti commented Feb 28, 2013

Thanks for your help.

ixti added a commit that referenced this pull request Feb 28, 2013
add ability to generate static gzip files
@ixti ixti merged commit f706dbe into envygeeks:master Feb 28, 2013
@ixti
Copy link
Contributor

ixti commented Mar 2, 2013

After some considerations - I have made gzip options to be either an array of mime-types that should be gzipped (text/css and application/javascript by default) or boolean false to disable gzipping. Thanks for your work once again.

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.

None yet

2 participants