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 Brotli to NGINX #35

Merged
merged 3 commits into from
Jul 28, 2022
Merged

Add Brotli to NGINX #35

merged 3 commits into from
Jul 28, 2022

Conversation

enoldev
Copy link
Contributor

@enoldev enoldev commented Jul 27, 2022

Fixes #21.

This PR contains:

  • A new Dockerfile that uses a multi-stage build to:
    (Stage 1) Compile from source Nginx, get the Brotli and generate the .so files for the modules.
    (Stage 2) By using the official nginx Docker image, the modules are moved to the corresponding directory.
  • Changes in the nginx.conf file to enable Brotli, along with the supported types.

NOTE: text/html is always compressed, but I have added it to the supported types, just in case this default behavior changes in the future.

@gruns
Copy link
Contributor

gruns commented Jul 27, 2022

awesome work @enolal826!! 🙌 🙌

  1. i cant tell from reading https://github.com/google/ngx_brotli whether responses are always compressed if the response mimetype matches one in brotli_types, or whether responses are only compressed if the response mimetype matches one in brotli_types and the requesting client announced they support brotli with the Content-Encoding: br, ... header

    we only want the latter. the vast majority of browsers support brotli, but ie 11 notably does not https://caniuse.com/brotli

  2. add gzip fallback compression when the browser doesn't support brotli. the same mimetype list of compressable filetypes should be used

  3. is the default brotli (and gzip) compression level of 6 best for us? probably. but i just want to make sure

NOTE: text/html is always compressed, but I have added it to the supported types, just in case this default behavior changes in the future.

👍

@DiegoRBaquero
Copy link
Collaborator

I'd remove text/html because of the annoying warning nginx: [warn] duplicate MIME type "text/html" in /etc/nginx/nginx.conf:52

I don't think they will remove it

@DiegoRBaquero DiegoRBaquero merged commit 40eb727 into main Jul 28, 2022
@DiegoRBaquero DiegoRBaquero deleted the enol/brotli branch July 28, 2022 16:14
@gruns
Copy link
Contributor

gruns commented Jul 28, 2022

any way to deduplicate the gzip_types and brotli_types variables? their values are identical. these:

b5e1cb2#diff-072cd79f6429749707ffb91c13810f82fa3ef6d0d9a8fddaa7c90e6e5c7bc4aeR43-R52

and

2f5182e#diff-072cd79f6429749707ffb91c13810f82fa3ef6d0d9a8fddaa7c90e6e5c7bc4aeR43-R52

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.

add brotli compression support
3 participants