imagemin / imagemin-pngquant

Imagemin plugin for `pngquant`
MIT License
316 stars 81 forks source link

Fix issue with `pngquant` and exit code 99 #71

Closed davet42 closed 4 years ago

davet42 commented 4 years ago

With recent updates to the image handling libraries in gatsby, routine image sharp operations started failing in builds. Here is an example issue in gatsby: https://github.com/gatsbyjs/gatsby/issues/26723

Whenever a pngquant invocation cannot complete the requested transform (based on combination of the output and of the specified quality parameters), the binary returns a code of 99. Perhaps because of unrelated updates in node.js packages, I found that the existing validation using the "error.code" element did not work. Instead, the check should be performed on "error.exitCode".

This pull request makes a one line change (and adds comments - not sure if they follow the standards of the team but are provided for extra clarity). Through additional instrumentation in index.js, I was able to determine that the "error.code" parameter comes back as "undefined" in some cases when running on Ubuntu. It also comes back as EPIPE, which is often a signal that the socket connection is no longer there. In any case, the error.exitCode parameter does always seem to be correct.

I've run this using the gatsby setup on my project with a range of jpg and png images, using a variety of quality values. All appear to work with the change.

Changed this code (literally just error.code to error.exitCode):

    const promise = subprocess
        .then(result => result.stdout) // eslint-disable-line promise/prefer-await-to-then
        .catch(error => {
            if (error.code === 99) {
                return input;
            }

            error.message = error.stderr || error.message;
            throw error;
        });

To this:

    const promise = subprocess
        .then(result => result.stdout) // eslint-disable-line promise/prefer-await-to-then
        .catch(error => {
            if (error.exitCode === 99) {
                return input;
            }

            error.message = error.stderr || error.message;
            throw error;
        });

I also changed the package version to 9.0.1 on logic that it is a non-breaking minor change.

davet42 commented 4 years ago

Looks good to me! (Gatsby maintainer here)

Forgive me - what is the best practice now? Should I close the pull request?

francamps commented 4 years ago

Are there any updates on this PR?

karlhorky commented 4 years ago

Forgive me - what is the best practice now? Should I close the pull request?

@davet42 I think a maintainer of imagemin-pngquant (such as @1000ch or @sindresorhus) needs to take a look and if it's ok, merge the pull request.

So keep it open!

SilencerWeb commented 4 years ago

@kevva @bradbaris maybe one of you could look at this PR, please? 🙂

sindresorhus commented 4 years ago

The problem here was caused by https://github.com/imagemin/imagemin-pngquant/commit/d285e92ba255aa072c8babec29dd9122cfb1a57a. It upgraded the execa dependency without taking a note of the breaking changes in execa v2: https://github.com/sindresorhus/execa/releases/tag/v2.0.0 (error.code was removed in favor of error.exitCode).

// @1000ch

sindresorhus commented 4 years ago

https://github.com/imagemin/imagemin-pngquant/releases/tag/v9.0.1

1000ch commented 4 years ago

@sindresorhus Ah, sorry and thank you