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

Prefer zlib over zopfli for gzip? #39

Closed
keeganstreet opened this issue Sep 19, 2019 · 7 comments

Comments

@keeganstreet
Copy link

commented Sep 19, 2019

There was some nice work done in #36 to make iltorb and node-zopfli-es optional.

But zopfli-compat.js is preferring zopfli over zlib, and printing a warning if node-zopfli-es is not installed. zopfli may create gzip assets 5% smaller than zlib, but it is much slower (~100x).

Since shrink-ray-current is for use at runtime, speed is very important and I would prefer to use zlib not zopfli. This would be consistent with brotli-compat.js which is preferring zlib over iltorb. Would you be open to this change, or alternatively silencing the warning Module "node-zopfli-es" was unavailable?

@Alorel

This comment has been minimized.

Copy link
Owner

commented Sep 23, 2019

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.

@Alorel Alorel self-assigned this Sep 23, 2019
@keeganstreet

This comment has been minimized.

Copy link
Author

commented Sep 23, 2019

Hi Alorel, that's OK. If we add an option the change might look something like this:

  • Add a boolean option called useZopfliForGzip which defaults to true
  • In index.js, move const zopfli = zopfliCompat(); inside the compression function so we can pass opts.useZopfliForGzip to zopfliCompat.
  • In zopfli-comat.js, return require('zlib') if useZopfliForGzip is false.

Would you like a PR for this?

@Alorel

This comment has been minimized.

Copy link
Owner

commented Sep 25, 2019

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.

@keeganstreet

This comment has been minimized.

Copy link
Author

commented Sep 26, 2019

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 😄

CodeIter added a commit to CodeIter/shrink-ray that referenced this issue Sep 27, 2019
Hope it help

Signed-off-by: Mohamed Amin Boubaker <mohamed.amin.02@gmail.com>
CodeIter added a commit to CodeIter/shrink-ray that referenced this issue Sep 27, 2019
`Uncaught ReferenceError: zopfli is not defined
Hope it work now!

Issue-ref: Alorel#39
Signed-off-by: Mohamed Amin Boubaker <mohamed.amin.02@gmail.com>
@CodeIter

This comment has been minimized.

Copy link

commented Sep 27, 2019

ok travis-ci test passed but COVERAGE DECREASED (-0.5%) TO 98.626% in file zopfli-compat.js

CodeIter added a commit to CodeIter/shrink-ray that referenced this issue Sep 27, 2019
Hope it work 100% now!

PR-ref: Alorel#40
Issue-ref: Alorel#39
Signed-off-by: Mohamed Amin Boubaker <mohamed.amin.02@gmail.com>
@CodeIter

This comment has been minimized.

Copy link

commented Sep 27, 2019

COVERAGE DECREASED (-0.3%) TO 98.898% in file zopfli-compat.js

//...
function getZopfliModule(useZopfliForGzip) {
    try {
      if(useZopfliForGzip){
        return require('node-zopfli-es');
      }
//...
@Alorel Alorel closed this in 1072f51 Sep 28, 2019
@Alorel

This comment has been minimized.

Copy link
Owner

commented Sep 28, 2019

Thanks for the contribution, published as 4.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.