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 issue with pngquant and exit code 99 #71

Merged
merged 7 commits into from
Sep 9, 2020

Conversation

davet42
Copy link
Contributor

@davet42 davet42 commented Sep 1, 2020

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: gatsbyjs/gatsby#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.

Copy link

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Looks good to me! (Gatsby maintainer here)

@davet42 davet42 closed this Sep 2, 2020
@davet42 davet42 reopened this Sep 2, 2020
@davet42
Copy link
Contributor Author

davet42 commented Sep 2, 2020

Looks good to me! (Gatsby maintainer here)

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

@francamps
Copy link

Are there any updates on this PR?

@karlhorky
Copy link

karlhorky commented Sep 7, 2020

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
Copy link

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

@sindresorhus
Copy link
Contributor

The problem here was caused by d285e92. 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 sindresorhus changed the title Fix to resolve issues in running pngquant binary when status code 99 returned (output intended to be just input) Fix issue with pngquant and exit code 99 Sep 9, 2020
@sindresorhus sindresorhus merged commit c8e1546 into imagemin:master Sep 9, 2020
@sindresorhus
Copy link
Contributor

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

@1000ch
Copy link
Contributor

1000ch commented Sep 15, 2020

@sindresorhus Ah, sorry and thank you

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.

8 participants