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 Type bug in WebpOptions. #3758

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

sho-xizz
Copy link
Contributor

@sho-xizz sho-xizz commented Aug 9, 2023

While use sharp to convert to WebP with TypeScript, I found the Type Error of WebpOptions.minSize.
it param is boolean, but Type specify as number.
So we had going to use @ts-ignore when use this option.

This PR corrects this bug.

@sho-xizz sho-xizz changed the title fix Type bug in webpOptions. fix Type bug in WebpOptions. Aug 9, 2023
@lovell
Copy link
Owner

lovell commented Aug 9, 2023

Thank you, this looks like a bug inherited from when the TypeScript definitions were imported from DefinitelyTyped. Are you able to add a test for this also, to help prevent regression?

@sho-xizz
Copy link
Contributor Author

sho-xizz commented Aug 9, 2023

Already there is minSize test as Support minSize and mixed webp options, but it looks like wrong.
So, I fix it param to boolean from number.

If this change is bad commit, could you tell me an example in one of your test cases I can base myself on to respect your guidelines, if any...?

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Brilliant, thank you, the existing (inherited) test case was wrong so thanks for fixing this too!

@lovell lovell added this to the v0.32.5 milestone Aug 9, 2023
@lovell lovell merged commit 87562a5 into lovell:main Aug 9, 2023
17 checks passed
lovell added a commit that referenced this pull request Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants