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

Support passing zlibOptions and brotliOptions #113

Merged
merged 2 commits into from
Jun 26, 2020

Conversation

SystemDisc
Copy link
Contributor

#106

This does not include tests or a benchmark - just wanted to quickly throw together a first-step in the direction of completing this issue: #106

This also does not include documentation, simple example is as follows:

  server.register(fastifyCompress, {
    brotliOptions: {
      params: {
        [zlib.constants.BROTLI_PARAM_MODE]: zlib.constants.BROTLI_MODE_TEXT, // useful for APIs that primarily return text
        [zlib.constants.BROTLI_PARAM_QUALITY]: 4, // default is 11, max is 11, min is 0
      },
    },
    zlibOptions: {
      level: 9, // default is 9, max is 9, min is 0
    }
  });

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

@mcollina
Copy link
Member

This is great, thanks! Can you add the tests and docs?

@SystemDisc
Copy link
Contributor Author

SystemDisc commented May 29, 2020

I will try to when I have time! I just wanted to get it started, so at least there's something to go off of in case someone else has more time. I'm currently working upwards of 90 hours per week. I just happen to be using this library, and saw this issue when I ran into a weird problem with Brotli compression. I disabled Brotli in the project I'm working on because it's not necessary. I wish I had more time to contribute. I love the open-source community. Great stuff!

@mcollina
Copy link
Member

Ouch, I'm sorry for your 90 hours. Thanks for the contribution!

@chasingSublimity
Copy link
Contributor

hi there! I can take a swing at adding docs and tests, if that's alright.

@Eomm
Copy link
Member

Eomm commented Jun 24, 2020

@chasingSublimity go for it! 👍

@chasingSublimity
Copy link
Contributor

Ok, here's a first pass. I'm new to Fastify, TypeScript, and zlib so happy to make any improvements you all suggest.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@SystemDisc
Copy link
Contributor Author

Does someone want to run benchmarks, just in case?

@mcollina
Copy link
Member

Not needed here, thanks for checking!

@mcollina mcollina merged commit 0007b90 into fastify:master Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants