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

feat: include only compressible mime types #761

Merged
merged 9 commits into from
Dec 20, 2022

Conversation

nathanchase
Copy link
Contributor

πŸ”— Linked issue

https://github.com/nuxt/framework/discussions/9626

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

The code for compressPublicAssets was double-compressing all image types (including .jpg, .png, .gif, .webp, etc.) when it really should only be compressible static file types that are text-based. I updated the code to use the following list of MIME types, and also added it to the docs for compressPublicAssets:

text/html
text/css
text/plain
text/xml
text/x-component
text/javascript
application/x-javascript
application/javascript
application/json
application/manifest+json
application/vnd.api+json
application/xml
application/xhtml+xml
application/rss+xml
application/atom+xml
application/vnd.ms-fontobject
application/x-font-ttf
application/x-font-opentype
application/x-font-truetype
image/svg+xml
image/x-icon
image/vnd.microsoft.icon
font/ttf
font/eot
font/otf
font/opentype

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

src/compress.ts Outdated Show resolved Hide resolved
src/compress.ts Outdated Show resolved Hide resolved
src/compress.ts Outdated Show resolved Hide resolved
src/compress.ts Outdated Show resolved Hide resolved
src/compress.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 17, 2022

Codecov Report

Merging #761 (3cb1219) into main (fc75d47) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 3cb1219 differs from pull request most recent head d5a9c72. Consider uploading reports for the commit d5a9c72 to get more accurate results

@@            Coverage Diff             @@
##             main     #761      +/-   ##
==========================================
+ Coverage   72.27%   72.30%   +0.03%     
==========================================
  Files          57       57              
  Lines        5154     5157       +3     
  Branches      556      555       -1     
==========================================
+ Hits         3725     3729       +4     
+ Misses       1419     1418       -1     
  Partials       10       10              
Impacted Files Coverage Ξ”
src/compress.ts 94.93% <100.00%> (+1.51%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pi0
Copy link
Member

pi0 commented Dec 19, 2022

Thanks for the PR @nathanchase and review @manniL @danielroe.

I think currently, docs is not consistent with implementation. We shall maintain an explicit list (array) of supported mimes. This also allows making it easier to customize by users whereas regex is harder to extend. Also we might include webp to the list as is text based.

@nathanchase
Copy link
Contributor Author

nathanchase commented Dec 19, 2022

@pi0 We could use https://www.npmjs.com/package/compressible, or maybe something similar that checks against a known list of compressible mime types.

@nathanchase
Copy link
Contributor Author

Here's a list that AWS uses: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/ServingCompressedFiles.html#compressed-content-cloudfront-file-types

@nathanchase
Copy link
Contributor Author

Also we might include webp to the list as is text based.

I was under the understanding that using Brotli/Gzip on .webp files would just increase the file size. I'm unable to find a clear answer to that question in my research, other than image/webp not appearing on any compressible type lists.

@pi0
Copy link
Member

pi0 commented Dec 19, 2022

compressible uses mime-db, which nitro already stubs into this.

AWS list, seems a great start πŸ‘πŸΌ Let's not over complicate this.

I was under the understanding that using Brotli/Gzip on .webp files would just increase the file size. I'm unable to find a clear answer to that question in my research, other than image/webp not appearing on any compressible type lists.

You are right. Totally my bad. I meant SVG πŸ€¦πŸΌβ€β™‚οΈ

@nathanchase
Copy link
Contributor Author

Take a look at the latest commits and see what you think. Should be good to go.

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ’―

@pi0 pi0 merged commit 378b8f9 into nitrojs:main Dec 20, 2022
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.

4 participants