Closed keeganstreet closed 5 years ago
Hi and sorry for the late reply.
I would be more keen on silencing the error or adding a config option for it. node-zopfli does tend to offer better compression than zlib, even if slightly, and I believe it should be favoured in most scenarios where you're not trying to squeeze out the most CPU savings as possible.
Hi Alorel, that's OK. If we add an option the change might look something like this:
useZopfliForGzip
which defaults to true
const zopfli = zopfliCompat();
inside the compression
function so we can pass opts.useZopfliForGzip
to zopfliCompat
.zopfli-comat.js
, return require('zlib')
if useZopfliForGzip
is false.Would you like a PR for this?
Yep, that looks good. And yes, if you'd be willing to make a PR that'd be amazing, else I could get round to it over the weekend.
I won't have time this week, but this is one of my next cards to pick up next week at work. No need to work on the weekend 😄
ok travis-ci test passed but COVERAGE DECREASED (-0.5%) TO 98.626% in file zopfli-compat.js
COVERAGE DECREASED (-0.3%) TO 98.898% in file zopfli-compat.js
//...
function getZopfliModule(useZopfliForGzip) {
try {
if(useZopfliForGzip){
return require('node-zopfli-es');
}
//...
Thanks for the contribution, published as 4.1.0
There was some nice work done in #36 to make
iltorb
andnode-zopfli-es
optional.But zopfli-compat.js is preferring
zopfli
overzlib
, and printing a warning ifnode-zopfli-es
is not installed.zopfli
may create gzip assets 5% smaller thanzlib
, but it is much slower (~100x).Since
shrink-ray-current
is for use at runtime, speed is very important and I would prefer to usezlib
notzopfli
. This would be consistent with brotli-compat.js which is preferringzlib
overiltorb
. Would you be open to this change, or alternatively silencing the warningModule "node-zopfli-es" was unavailable
?