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

fix: default brotliOptions does not applied to createBrotliCompress and createBrotliDecompress #284

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

stanleyxu2005
Copy link
Contributor

@stanleyxu2005 stanleyxu2005 commented Mar 26, 2024

The problem

I encounted a remarkable performance slow down after migrating from Koa.
Although the doc mentions fastify/compress already sets the compression level to 4, but it takes too long to compress (3MB for 5 sec) a json. I guess it is configured unexpectedly by default.

After some investigation, it could be related to the previous merge request #278.
Unless I explicitly set the compression level to 4, I will get 11 (BROTLI_DEFAULT_QUALITY).

The root cause

The compression level is a member of brotliOptions.params not brotliOptions. https://nodejs.org/api/zlib.html#class-brotlioptions

And unfortunately the global default option is overwritten by custom option afterwards https://github.com/fastify/fastify-compress/blob/master/index.js#L129

The change

  1. Proposed a fix with additional test cases included.
  2. Added a new test case to ensure with default options, response will be compressed with level 4.
  3. Changed the existing test case to speicfy the compression level to 8 and check if it is compressed expectedly.

Loop in author of previous commit @kahlstrm as well.

Checklist

gurgunday
gurgunday previously approved these changes Mar 26, 2024
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

@gurgunday
Copy link
Member

There is a type error... can you take a look?

@stanleyxu2005
Copy link
Contributor Author

stanleyxu2005 commented Mar 27, 2024

There is a type error... can you take a look?

@gurgunday I tried to understand the error, but have no clue why it is failed.

It expects to throw an type error for inject(). I dont see the callback is reachable. Strangely, I checkout the master branch it shows the same error, but the pipeline for previous commits on GitHub are successful. Could you help to figure out?

@stanleyxu2005
Copy link
Contributor Author

After some more code reading, I made a follow-up change. The compress level will be set to 4 for the global option only. If route has its own compression options, it will be merged with global options anway. No need to double overwrite.

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. but for some reasons our types tests are failing.

(This can’t land otherwise, and it seems not independent).

@stanleyxu2005
Copy link
Contributor Author

I'm thinking if https://github.com/fastify/fastify-compress/blob/master/types/index.test-d.ts#L101-L112 is needed. I'm prefer to remove them

@gurgunday
Copy link
Member

Can you try this?

appWithoutGlobal.inject(
  {
    method: 'GET',
    path: '/throw-a-ts-arg-error',
    headers: {
      'accept-encoding': 'gzip'
    }
  },
  (err) => {
    expectType<Error | undefined>(err) // add undefined
  }
)

@stanleyxu2005
Copy link
Contributor Author

stanleyxu2005 commented Mar 27, 2024

@gurgunday @mcollina by changing expectType<Error>(err) to expectType<Error | undefined>(err), all test errors are gone at my local env. Committed a new change, and hope all checks will be passed soon. 😆

@gurgunday
Copy link
Member

gurgunday commented Mar 27, 2024

Yeah seems like ts(d) behavior changed

I will release a new version of this package

@stanleyxu2005
Copy link
Contributor Author

Yeah seems like ts behavior changed

I will release a new version of this package

Indeed. It's an interesting test error.

Thank you both for reviewing the code change.

This project is quite well coded and documented. I'm glad that I contributed some bugfix for it.👍

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

@gurgunday gurgunday merged commit 46ce8f9 into fastify:master Mar 27, 2024
19 checks passed
@stanleyxu2005 stanleyxu2005 deleted the fix/default-brotlioptions branch March 27, 2024 11:41
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.

3 participants