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 setting compression level for brotliCompress #209

Merged
merged 2 commits into from
Oct 26, 2020

Conversation

180254
Copy link
Contributor

@180254 180254 commented Oct 24, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Fixes #154.

Breaking Changes

No

Additional Info

No

@jsf-clabot
Copy link

jsf-clabot commented Oct 24, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Maybe we should fix it in code too?

@180254
Copy link
Contributor Author

180254 commented Oct 24, 2020

What fixes would you like to see in the code? It comes to my mind that different default parameters may be passed depending on the algorithm. Do you see any other changes needed?

@alexander-akait
Copy link
Member

I mean this https://github.com/webpack-contrib/compression-webpack-plugin/blob/master/src/index.js#L70, we should always set to maximum, for brotly too

@180254 180254 changed the title docs: fixed setting compression level for brotliCompress. fix: settings compression level for brotliCompress. Oct 24, 2020
@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #209 into master will decrease coverage by 12.10%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #209       +/-   ##
===========================================
- Coverage   98.77%   86.66%   -12.11%     
===========================================
  Files           8        4        -4     
  Lines         326      165      -161     
  Branches       92       48       -44     
===========================================
- Hits          322      143      -179     
- Misses          2       20       +18     
  Partials        2        2               
Impacted Files Coverage Δ
src/index.js 89.23% <50.00%> (-9.21%) ⬇️
src/Webpack5Cache.js 0.00% <0.00%> (-100.00%) ⬇️
...ack-plugin/compression-webpack-plugin/src/index.js
...bpack-plugin/compression-webpack-plugin/src/cjs.js
...in/compression-webpack-plugin/src/Webpack5Cache.js
...in/compression-webpack-plugin/src/Webpack4Cache.js

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6aa8e38...436ec3f. Read the comment docs.

…ib#154)

Signed-off-by: Adrian Pedziwiatr <180254@users.noreply.github.com>
…k-contrib#154)

Signed-off-by: Adrian Pedziwiatr <180254@users.noreply.github.com>
@180254 180254 changed the title fix: settings compression level for brotliCompress. fix: settings compression level for brotliCompress Oct 24, 2020
@180254 180254 changed the title fix: settings compression level for brotliCompress fix: set proper compression level for brotliCompress Oct 25, 2020
@180254 180254 changed the title fix: set proper compression level for brotliCompress fix: fix setting compression level for brotliCompress (#154) Oct 25, 2020
@180254 180254 changed the title fix: fix setting compression level for brotliCompress (#154) fix setting compression level for brotliCompress Oct 25, 2020
@180254
Copy link
Contributor Author

180254 commented Oct 25, 2020

I think it's done now. Please check this PR again.

@alexander-akait alexander-akait merged commit 483f328 into webpack-contrib:master Oct 26, 2020
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.

Incorrect Brotli example
3 participants